998454 - 12-14% tart regression for tart on windows 8 on beta/aurora (Firefox 29/30) as a result of bug bug 477948

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

on mozilla-beta:
12.7% win8 tart regression

on mozilla-aurora
13.9% win8 tart regression

I played whack a mole with backouts on beta and when the patch for bug 477948 was backed out we went back to normal:
https://tbpl.mozilla.org/?tree=Try&rev=be431ebd29a0

in bug 990163 we have a 6% tart regression on linux/linux64 which was marked as wontfix.  Please verify this is acceptable to ship with.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

The only change for Windows is changing keyhole-forward-clip-path from objectBoundingBox to userSpaceOnUse and the respective SVG path changes.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

some reason this is causing a large enough regression on beta/aurora.  I know this landed on fx-team much earlier- could there be some other changes on trunk branches which has minimized/reduced this change?

Comment 3

[Dão Gottwald [:dao]]

(In reply to Joel Maher (:jmaher) from comment #2)
> could there be some other changes on trunk branches which has minimized/reduced this change?

Hard to imagine. This shouldn't really affect tart on Win8 at all.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

On Mozilla-Beta, you can see the jump here:
http://graphs.mozilla.org/graph.html#tests=[[293,53,31]]&sel=none&displayrange=30&datatype=running

Here is the commit that caused the pain:
http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=40db75f04aea&tochange=f1c211a4714d

I did verify that we were looking at the specific commit and not dealing with multiple commits and job coalescing:
https://tbpl.mozilla.org/?tree=Mozilla-Beta&fromchange=f91bdb05883b&tochange=679aa869f39f&jobname=WINNT%206.2%20mozilla-beta%20pgo%20talos%20svgr  <- shows we have win8 tart results for all builds

I then worked with mconley on irc and looked at each revision, we found 3 that might be problematic and I backed them out on try server:
https://tbpl.mozilla.org/?tree=Try&rev=be431ebd29a0

The values for tart on beta go like this:
* original      - ~4.5
* slight bump   - ~4.6
* this bug      - ~5.4
* other commits - ~5.1
* backout/try   - ~4.6

when I did the try run, I rolled back to the changeset right after this landed so I would avoid any confusion with future patches which landed a couple days later.

It is clearly this specific patch.  Is there a chance that we are using changes on windows and not realizing it?  Why is this windows 8 only?

Comment 5

[:Gijs (he/him)]

(In reply to Joel Maher (:jmaher) from comment #4)
> I then worked with mconley on irc and looked at each revision, we found 3
> that might be problematic and I backed them out on try server:
> https://tbpl.mozilla.org/?tree=Try&rev=be431ebd29a0

This backs out more than just 871c60982cac (bug 477948), as the mq patchname suggests - it also backs out bug 989466, which is altogether more likely as having caused the regression, as it actually altered the clip path - bug 477948 did not change anything on Windows apart from renaming the clip path.

Interestingly, bug 989466 was essentially a revert of an earlier commit - did we see a TART improvement for that? (bug 893661)

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

so it looks like I did back out those changes as well, is there something else I should do to reduce my patch?  I had so many conflicts while backing out bug 477948 that I did what I thought was right without knowing anything about the code.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

I keep confusing myself, I looked at my patch for backing out bug 477048 and it appears to be correct without the changes in bug 989466.

I still assert this is the patch from bug 477048.

Are we fine taking a 12% regression on tart when we release next week?

Comment 8

[Avi Halachmi (:avih)]

Do we have real alternatives?

This will have to pass IMO. It's too late to back it out and it's not breaking anything.

Comment 9

[Avi Halachmi (:avih)]

Just to make it clear, the regression is on Firefox 29 (due to relatively late uplift). This also regresses on Firefox 30, but apparently not on trunk. jmaher suggests that it might have been mitigated on trunk.

Comment 10

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #9)
> ... jmaher suggests that it might have been mitigated on trunk.

Mike, could you shed some light on this, or think of anyone else who might?

Comment 11

[Mike de Boer [:mikedeboer]]

(In reply to Avi Halachmi (:avih) from comment #10)
> (In reply to Avi Halachmi (:avih) from comment #9)
> > ... jmaher suggests that it might have been mitigated on trunk.
> 
> Mike, could you shed some light on this, or think of anyone else who might?

I *think* other things landed on trunk that result in a TART improvement, not directly related to the keyhole bug.

However, the only thing that bug 477948 changed for Windows 8 (any Windows version, TBH) is the ID of the clip-path.
Later we went apeshit on the navbar with bug 893661, and so I ended up reverting the clip-path changes made there. It's very unfortunate that these two rather big modifications landed in such quick succession, making it rather difficult for me to analyze this and other regressions.

All that aside, the only change left standing for the Windows theme is the clip-path ID change. Do we have SVG caching enabled? If so, did we get a cache-miss, which might explain the regression? mconley would know.
Other than that, it's all Linux and nothing but.

Comment 12

[Avi Halachmi (:avih)]

Considering comment 11 together with the fact that this regression disappeared on trunk, I'm closing as wontfix. If anyone reading this thinks it could be fixed for Firefox 30 with little risk, please file a new bug and block this bug.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

WONTFIX. So, we will not tracking it.

Comment 14

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

(In reply to Mike de Boer [:mikedeboer] from comment #11)

> All that aside, the only change left standing for the Windows theme is the
> clip-path ID change. Do we have SVG caching enabled? If so, did we get a
> cache-miss, which might explain the regression? mconley would know.
> Other than that, it's all Linux and nothing but.

We do have SVG caching enabled, but I'm not sure if we'd somehow be hitting a cache miss here. Hard for me to say.

And if, like it sounds, this regression has mysteriously gone away in 31...well, maybe we should just live with it until then. Probably better than burning more engineering time on it at this point, imo.

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to Mike Conley (:mconley) from comment #14)
> And if, like it sounds, this regression has mysteriously gone away in
> 31...well, maybe we should just live with it until then.

Yeah, it did and I think I can also live with this knowing that 31 will be good again.

> Probably better
> than burning more engineering time on it at this point, imo.

Aye. Thanks, Mike!