1080770 - 10% OSX Cart regression on Aurora (v.34) Oct 6, from push da372ef9b8ef

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

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

Here are some charts:
http://graphs.mozilla.org/graph.html#tests=%5B%5B309,52,21%5D,%5B309,52,24%5D%5D&sel=1412275323038,1412880123038&displayrange=7&datatype=running

I don't see any improvements on Aurora, just these two osx regressions.  I have done some retriggers to help verify this is accurate:
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&fromchange=95cf39085d65&tochange=19072e3f4c0b&jobname=talos%20svgr

the culprit is:
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=da372ef9b8ef

Comment 1

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

as patch authors, can you please take a moment to look at if your patch would affect CART:
https://wiki.mozilla.org/Buildbot/Talos/Tests#TART.2FCART

Comment 2

[Nicolas B. Pierron [:nbp]]

changeset 8dade5654081 sounds unlikely to cause regressions on Customize Animation Regression Test.  As this is a backout of a code clean-up patch, and I would not expect to have recurrent AsmJS compilation during animations.

I do not know much about gfx, but my blind guess would be toward Bug 1067191 - Part 1.

Comment 3

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

thanks :nbp, I appreciate your time.  Bug 1067191 does look suspicious.  That patch (or a similar one) landed on trunk on 09-26, I didn't see a bump at all then.  That doesn't mean that it isn't the culprit, it just makes it harder to put a+b together.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I would be quite shocked if bug 1070764 or bug 1057541 affected this, since none of the code they touched is exercised during this test.

Comment 5

[Victor Porof [:vporof][:vp]]

I don't think the profiler backwards compatibility patch can affect this.

Comment 6

[Alan K [:ack]]

I don't see how the code I landed could cause this.

Comment 7

[shawn ku [:sku]]

My patch (https://hg.mozilla.org/releases/mozilla-aurora/rev/2bc3a7557573) is SIM card related patch, should not impact CART.

Comment 8

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

That leaves Seth and he has the most suspecting patch.

Comment 9

[Seth Fowler [:seth] [:s2h]]

That code fixed a bug in how we compute the destination rect when drawing images. It just changed some math; it didn't add any new heavyweight operations. It could certainly cause us to draw more pixels in certain situations, but those are situations in which our rendering was previously buggy.

In short: if that patch caused a performance regression, it's because we were previously producing incorrect rendering results.

I don't think there's anything actionable for me in this bug. Performance is important, but if my patch is the cause, I don't see a path to improving this benchmark that wouldn't cause a regression in correctness.

Comment 10

[Avi Halachmi (:avih)]

(In reply to Seth Fowler [:seth] from comment #9)
> ...
> In short: if that patch caused a performance regression, it's because we
> were previously producing incorrect rendering results.

Would it be possible to notice these changes/improvements visually? e.g. during the Customize animation (which is what supposedly regressed here)?

Comment 11

[Seth Fowler [:seth] [:s2h]]

(In reply to Avi Halachmi (:avih) from comment #10)
> Would it be possible to notice these changes/improvements visually? e.g.
> during the Customize animation (which is what supposedly regressed here)?

Quite possibly. The issue that was fixed in that bug is that we were wrongly rounding the scale we were applying at one point during the computation where we try to snap images to the pixel grid. So on some code paths, primarily used by CSS backgrounds and border-image, we were scaling incorrectly - we'd get a 1x scale instead of a 1.4x scale, for example.

If my patch regressed the CART animation, then it was almost certainly triggering that bug at some point during the animation, which would likely be visible as something like a CSS image background not filling the entire area that it's supposed to.

In the worst case, we were originally rounding some image background to a 1x scale, but now without rounding we're at a larger scale like 1.4x, which means that instead of applying no scaling we're now applying HQ scaling on the CPU (since we recently switched to CPU-based scaling on OS X).

Comment 12

[Avi Halachmi (:avih)]

As far as I can tell the only possibly BG image during customize is the blue background pattern around the menu on the right. Due to its mostly generic pattern I don't think it's possible to notice changes in alignments, and TBH, I don't recall noticeably incorrect filling in past versions.

That's not to say that this patch is bad. I just hoped such meaningful fix could be noticed on cases where it regressed performance.

Is there a test case which shows what the bug fixes?

Comment 13

[Seth Fowler [:seth] [:s2h]]

(In reply to Avi Halachmi (:avih) from comment #12)
> That's not to say that this patch is bad. I just hoped such meaningful fix
> could be noticed on cases where it regressed performance.

It's extremely obvious in other situations. It may not be in this one, unfortunately.

> Is there a test case which shows what the bug fixes?

Sure, see the STR of bug 1067191, or the test files added in patch 2.

Comment 14

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #13)
> Sure, see the STR of bug 1067191, or the test files added in patch 2.

The reporter provided a screenshot of the STR results, actually, so you don't even need to make a build without the patch.

Comment 15

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #11)
> In the worst case, we were originally rounding some image background to a 1x
> scale, but now without rounding we're at a larger scale like 1.4x, which
> means that instead of applying no scaling we're now applying HQ scaling on
> the CPU (since we recently switched to CPU-based scaling on OS X).

BTW, I just realized that I misspoke here. We only do HQ scaling for *downscales*, so to trigger CPU-based scaling, it would have to be a scale of less than 1.0.

Comment 16

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

this is 6+ months old, closing as wontfix since we shipped already.