Closed Bug 880620 Opened 7 years ago Closed 6 years ago

[10.6] Talos tresize chromez regression of 15-20% on UX 1019becd7a2a from OS X titlebar patches

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: MattN, Assigned: mstange)

References

(Blocks 1 open bug, )

Details

(Keywords: perf, Whiteboard: [Australis:P?])

Attachments

(3 files, 1 obsolete file)

There is a 15-20% increase on Snow Leopard (10.6) only. 10.7 and 10.8 have a negligible change compared to m-c. See the URL for the graph. Ignore the fact that UX was better than m-c before the landing because that was due to measurement error (see bug 880554 comment 2).

Pushlog: http://hg.mozilla.org/projects/ux/pushloghtml?changeset=1019becd7a2a
Hey Markus,

Not sure if this regression will block our landing in m-c, as it's 10.6 only... even still, do you have any idea what might be going on here?
Flags: needinfo?(mstange)
Bumping these to M8, since M7 finished yesterday.
Whiteboard: [Australis:M7] → [Australis:M8]
Marcus -- any update here? We're getting close to looking at landing!
I haven't had a chance to look at this yet, unfortunately. And I won't have access to my 10.6 machine until Monday.
Flags: needinfo?(mstange)
I think we really need to understand this better, but a regression on just 10.6 on just this measurement is probably shippable.
Whiteboard: [Australis:M8] → [Australis:M8][Australis:P2]
I've done some profiling and the results are unsurprising: it's exactly the code which I added in bug 676241 which is responsible for the slowdown. More specifically: 
 - UpdateTitlebarImageBuffer(): buffer allocation / deallocation, highlight line gradient drawing
 - MaybeDrawTitlebar(): texture scratch buffer allocation, copy of mTitlebarImageBuffer into the scratch buffer, texture object allocation, texture deletion

I'm testing multiple remedies for the slowdowns now. For the texture deletion part, the changes from bug 882523 should already help because that causes us to snap texture size to the next power of two which will result in fewer reallocations.
Attached patch part 1: manual gradient drawing (obsolete) — Splinter Review
Attachment #770732 - Flags: review?(matt.woodrow)
With the patch I landed in bug 882523 plus these two patches, the regression seems to be fixed completely: https://tbpl.mozilla.org/?tree=Try&rev=d09d5ae46e03
Attachment #770733 - Flags: review?(matt.woodrow) → review+
Comment on attachment 770732 [details] [diff] [review]
part 1: manual gradient drawing

I don't know this code well enough to review, sorry.
Attachment #770732 - Flags: review?(matt.woodrow)
Attachment #770732 - Flags: review?(bgirard)
Comment on attachment 770732 [details] [diff] [review]
part 1: manual gradient drawing

Review of attachment 770732 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsChildView.mm
@@ +2091,5 @@
>  static void
>  DrawTitlebarHighlight(NSSize aWindowSize, CGFloat aRadius, CGFloat aDevicePixelWidth)
>  {
> +  if (aRadius <= 0) {
> +    return;

Just curious does this get it?

@@ +2114,5 @@
> +  for (CGFloat y = 0; y < aRadius; y += aDevicePixelWidth) {
> +    CGFloat t = y / aRadius;
> +    [[NSColor colorWithDeviceWhite:1.0 alpha:0.4 * (1.0 - t)] set];
> +    NSRectFill(NSMakeRect(0, y, aWindowSize.width, aDevicePixelWidth));
> +  }

This is faster? I think this is worth a comment in the code base and maybe an explanation of your reasoning. I imagine you tested this and found it to be faster then NSGradient but I'd still like to hear it. Does NSGradient not have a fast path for this type of gradient?
Attachment #770732 - Flags: review?(bgirard)
part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/96141d8ae195
Whiteboard: [Australis:M8][Australis:P2] → [Australis:M8][Australis:P2][leave open]
Just an FYI that with other Australis perf fixes, UX is faster than m-c on tresize so I'm dropping the priority as it won't currently block shipping.
Whiteboard: [Australis:M8][Australis:P2][leave open] → [Australis:M8][Australis:P3][leave open]
Whiteboard: [Australis:M8][Australis:P3][leave open] → [Australis:P5][leave open]
Whiteboard: [Australis:P5][leave open] → [Australis:P?][leave open]
Matt, will we leave this open forever or can it collapse in another bug?
Flags: needinfo?(mnoorenberghe+bmo)
(In reply to Mike de Boer [:mikedeboer] from comment #15)
> Matt, will we leave this open forever or can it collapse in another bug?

Markus is the one who set [leave open] so over to him.
Flags: needinfo?(MattN+bmo) → needinfo?(mstange)
This app draws gradients both ways and prints the speedup factor. Resize the window to trigger drawing.
Flags: needinfo?(mstange)
Attachment #8348824 - Attachment is patch: false
(In reply to Benoit Girard (:BenWa) from comment #11)
> Comment on attachment 770732 [details] [diff] [review]
> part 1: manual gradient drawing
> 
> Review of attachment 770732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/cocoa/nsChildView.mm
> @@ +2091,5 @@
> >  static void
> >  DrawTitlebarHighlight(NSSize aWindowSize, CGFloat aRadius, CGFloat aDevicePixelWidth)
> >  {
> > +  if (aRadius <= 0) {
> > +    return;
> 
> Just curious does this get it?

Apparently it does not get hit. I don't remember why I added this, so I've removed it now.

> 
> @@ +2114,5 @@
> > +  for (CGFloat y = 0; y < aRadius; y += aDevicePixelWidth) {
> > +    CGFloat t = y / aRadius;
> > +    [[NSColor colorWithDeviceWhite:1.0 alpha:0.4 * (1.0 - t)] set];
> > +    NSRectFill(NSMakeRect(0, y, aWindowSize.width, aDevicePixelWidth));
> > +  }
> 
> This is faster? I think this is worth a comment in the code base and maybe
> an explanation of your reasoning. I imagine you tested this and found it to
> be faster then NSGradient but I'd still like to hear it. Does NSGradient not
> have a fast path for this type of gradient?

It is faster. The test app in attachment 8348824 [details] prints speedup factors between 5x and 15x (more for bigger window sizes). I've added a comment.
Attachment #770732 - Attachment is obsolete: true
Attachment #8348825 - Flags: review?(bgirard)
(In reply to Mike de Boer [:mikedeboer] from comment #15)
> will we leave this open forever or can it collapse in another bug?

I wanted to leave it open for landing part 1. The performance regression is fixed, but if this can get us another percent or so, we should still take it.
Comment on attachment 8348825 [details] [diff] [review]
part 1: manual gradient drawing

Review of attachment 8348825 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsChildView.mm
@@ +2178,5 @@
>    [path appendBezierPathWithRoundedRect:pathRect xRadius:innerRadius yRadius:innerRadius];
>    [path addClip];
>  
>    // Now we fill the path with a subtle highlight gradient.
> +  // We don't use NSGradient for performance reasons, see bug 880620.

IMO we should summarize this in the code base. I think it's very poor when we read the code and have to look up several bugs (and read 100s of comments) to find a few pieces of information. It should be summarized in a comment when the results are non obvious and the hg blame will provide a link to the bug for more details.
Attachment #8348825 - Flags: review?(bgirard) → review+
How about:
// We don't use NSGradient because it's 5x to 15x slower than the manual fill,
// as indicated by the performance test in bug 880620.
?
Sounds great, ship it!
This resulted in a 6.7% tresize improvement on 10.6:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/F-giVd45udY

The regression from bug 676241 is probably fixed now.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [Australis:P?][leave open] → [Australis:P?]
Depends on: 1058713
You need to log in before you can comment on or make changes to this bug.