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

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
4 years ago
3 years ago

People

(Reporter: jmaher, Unassigned)

Tracking

({perf, regression})

Trunk
All
Mac OS X
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [talos_regression])

(Reporter)

Comment 1

4 years ago
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
Flags: needinfo?(vporof)
Flags: needinfo?(sku)
Flags: needinfo?(seth)
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(akligman)
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.
Flags: needinfo?(nicolas.b.pierron)
(Reporter)

Comment 3

4 years ago
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.
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.
Flags: needinfo?(bzbarsky)
I don't think the profiler backwards compatibility patch can affect this.
Flags: needinfo?(vporof)

Comment 6

4 years ago
I don't see how the code I landed could cause this.
Flags: needinfo?(akligman)

Comment 7

4 years ago
My patch (https://hg.mozilla.org/releases/mozilla-aurora/rev/2bc3a7557573) is SIM card related patch, should not impact CART.
Flags: needinfo?(sku)
(Reporter)

Comment 8

4 years ago
That leaves Seth and he has the most suspecting patch.
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.
Flags: needinfo?(seth)
(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)?
(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).
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?
(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.
(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.
(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.
(Reporter)

Comment 16

3 years ago
this is 6+ months old, closing as wontfix since we shipped already.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.