588664 - Speed up nsChildView::GetDPI

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 573890 (probably) caused a 5% Tp4 regression. Profiling shows that CGDisplayScreenSize accounts for 2.2% of a Tp3 run on my machine (we only call it once per document, basically). This is probably the cause of the regression.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's bug 537890.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes it.

Comment 3

[Josh Aas]

Comment on attachment 467285 [details] [diff] [review]
fix

> float
> nsChildView::GetDPI()
> {
>   NSWindow* window = [mView window];
...
>+  if ([window isKindOfClass:[BaseWindow class]]) {
>+    return [(BaseWindow*)window getDPI];
>+  }

You might want to null check "window" here. It won't crash if it is null but the result from sending messages to it might be unexpected.

>+  CGFloat heightMM = CGDisplayScreenSize(displayID).height;
>+  size_t heightPx = CGDisplayPixelsHigh(displayID);

In all of our Mac OS X code we try to prefix Mac OS X framework C calls with "::" ("::CGDisplay..."). Makes it easier to tell quickly whether or not a function belongs to us or the OS.

>+  return (heightPx / scaleFactor) / (heightMM / MM_PER_INCH_FLOAT);

Are you sure "heightMM" will never be zero here? If it is we'll crash with divide by zero. Same for "scaleFactor".

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #3)
> You might want to null check "window" here. It won't crash if it is null but
> the result from sending messages to it might be unexpected.

OK. (How can a view have a null window?)

> >+  CGFloat heightMM = CGDisplayScreenSize(displayID).height;
> >+  size_t heightPx = CGDisplayPixelsHigh(displayID);
> 
> In all of our Mac OS X code we try to prefix Mac OS X framework C calls with
> "::" ("::CGDisplay..."). Makes it easier to tell quickly whether or not a
> function belongs to us or the OS.

OK.

> >+  return (heightPx / scaleFactor) / (heightMM / MM_PER_INCH_FLOAT);
> 
> Are you sure "heightMM" will never be zero here? If it is we'll crash with
> divide by zero. Same for "scaleFactor".

I think it would be extremely antisocial for Apple to return zero for those values, but I'll check them anyway.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Comment 6

[Josh Aas]

> OK. (How can a view have a null window?)

When it is detached, not a subview of any window.

> > Are you sure "heightMM" will never be zero here? If it is we'll crash with
> > divide by zero. Same for "scaleFactor".
> 
> I think it would be extremely antisocial for Apple to return zero for those
> values, but I'll check them anyway.

I agree it wouldn't be likely or nice, but the possibility is there and we've seen worse.

Comment 7

[Markus Stange [:mstange]]

(In reply to comment #3)
> Comment on attachment 467285 [details] [diff] [review]
> fix
> 
> > float
> > nsChildView::GetDPI()
> > {
> >   NSWindow* window = [mView window];
> ...
> >+  if ([window isKindOfClass:[BaseWindow class]]) {
> >+    return [(BaseWindow*)window getDPI];
> >+  }
> 
> You might want to null check "window" here. It won't crash if it is null but
> the result from sending messages to it might be unexpected.

isKindOfClass returns a BOOL and as such is guaranteed to return NO when called on nil.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/5a3b5dbd2929