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

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Matthew N. [:MattN]]

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

Comment 1

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Bumping these to M8, since M7 finished yesterday.

Comment 3

[Justin Dolske [:Dolske]]

Marcus -- any update here? We're getting close to looking at landing!

Comment 4

[Markus Stange [:mstange]]

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.

Comment 5

[Justin Dolske [:Dolske]]

I think we really need to understand this better, but a regression on just 10.6 on just this measurement is probably shippable.

Comment 6

[Markus Stange [:mstange]]

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.

Comment 7

[Markus Stange [:mstange]]

Attachment: part 1: manual gradient drawing

Comment 8

[Markus Stange [:mstange]]

Attachment: part 2: don't allocate scratch buffer for mTitlebarImageBuffer texture upload

Comment 9

[Markus Stange [:mstange]]

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

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 770732 [details] [diff] [review]
part 1: manual gradient drawing

I don't know this code well enough to review, sorry.

Comment 11

[Benoit Girard (:BenWa)]

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?

Comment 12

[Markus Stange [:mstange]]

part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/96141d8ae195

Comment 13

[Matthew N. [:MattN]]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96141d8ae195

Comment 15

[Mike de Boer [:mikedeboer]]

Matt, will we leave this open forever or can it collapse in another bug?

Comment 16

[Matthew N. [:MattN]]

(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.

Comment 17

[Markus Stange [:mstange]]

Attachment: test app to compare drawing performance

This app draws gradients both ways and prints the speedup factor. Resize the window to trigger drawing.

Comment 18

[Markus Stange [:mstange]]

Attachment: 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.

Comment 19

[Markus Stange [:mstange]]

(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 20

[Benoit Girard (:BenWa)]

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.

Comment 21

[Markus Stange [:mstange]]

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.
?

Comment 22

[Benoit Girard (:BenWa)]

Sounds great, ship it!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e7eb1a98175

Comment 24

[Markus Stange [:mstange]]

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.