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

RESOLVED FIXED in mozilla29

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: MattN, Assigned: mstange)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla29
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P?], URL)

Attachments

(3 attachments, 1 obsolete attachment)

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!
(Assignee)

Comment 4

4 years ago
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]
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
Created attachment 770732 [details] [diff] [review]
part 1: manual gradient drawing
Attachment #770732 - Flags: review?(matt.woodrow)
(Assignee)

Comment 8

4 years ago
Created attachment 770733 [details] [diff] [review]
part 2: don't allocate scratch buffer for mTitlebarImageBuffer texture upload
Attachment #770733 - Flags: review?(matt.woodrow)
(Assignee)

Comment 9

4 years ago
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)
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 12

4 years ago
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]
https://hg.mozilla.org/mozilla-central/rev/96141d8ae195
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)
(Assignee)

Comment 17

4 years ago
Created attachment 8348824 [details]
test app to compare drawing performance

This app draws gradients both ways and prints the speedup factor. Resize the window to trigger drawing.
Flags: needinfo?(mstange)
(Assignee)

Updated

4 years ago
Attachment #8348824 - Attachment is patch: false
(Assignee)

Comment 18

4 years ago
Created attachment 8348825 [details] [diff] [review]
part 1: manual gradient drawing

(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)
(Assignee)

Comment 19

4 years ago
(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+
(Assignee)

Comment 21

4 years ago
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!
https://hg.mozilla.org/mozilla-central/rev/3e7eb1a98175
(Assignee)

Comment 24

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(Assignee)

Updated

4 years ago
Whiteboard: [Australis:P?][leave open] → [Australis:P?]
(Assignee)

Updated

3 years ago
Depends on: 1058713
You need to log in before you can comment on or make changes to this bug.