Closed Bug 990233 Opened 6 years ago Closed 6 years ago

5.16% tresize regression on osx 10.6 seen on inbound

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jmaher, Assigned: bjacob)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

Hm, that's unfortunate --- especially as I'm the reviewer for both patches in the regression range ;-)

http://hg.mozilla.org/integration/mozilla-inbound/rev/fed5c6d9c1da
http://hg.mozilla.org/integration/mozilla-inbound/rev/733a0129c739

I think I'll just back them out both for now, which is probably more brutal than needed, but at least allows us to take our time to investigate. I'm in Taipei for a work week at the moment. Needinfo'ing the author to ensure he knows about that.
Flags: needinfo?(gal)
When in doubt back out. I have a hard time seeing either bug impacting tresize but we can simply re-land them 1 by 1 and see what happens.
Flags: needinfo?(gal)
OK, backing out.

Note that the fact that this is reported only on 10.6 and not on 10.8 might contain a clue. The last time (see bug 950113) that we had a gfx/gl patch regress 10.6 performance specifically, it was because it was introducing some redundant GL calls that the 10.6 GL libraries were not able to optimize away, but 10.8 libraries were. So I would check for redundant GL calls.
Actually hold that thought. 10.6 only regression I am ok with carrying actually. What do you think?
Ok, I am pretty sure that this was caused by the GLDrawRectHelper patch, so please leave the other one in place.

Here is whats going on:

I removed VBOArena and I am allocating a VBO on the spot and free it right after because we didn't have any evidence that it actually makes a difference to use VBOArena. I guess we have proof now.

So back out this one and I will add VBOArena back to GLDrawRectHelper.
Sorry, I didn't see these last two comments in time; backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a63f5694f562

(In reply to Andreas Gal :gal from comment #4)
> Actually hold that thought. 10.6 only regression I am ok with carrying
> actually. What do you think?

Like I said in comment 3, we've seen a 10.6-specific GL regression before that was actually pointing at redundant calls worth catching; for that alone, I believe that we should care.

(In reply to Andreas Gal :gal from comment #5)
> Ok, I am pretty sure that this was caused by the GLDrawRectHelper patch, so
> please leave the other one in place.

Sorry, too late; you have my r+ to reland with or without the change you describe.
No worries, backing out is never wrong :) We will just reland.
Note that trychooser will also happily build a try command for running tresize on 10.6, which might save you the hassle of another backout.
In general, how much do we want to care about small OS X 10.6-only regressions as indicated by our perf-tests framework?

If it's possibly an indication of a more global issue - e.g. redundancies, then it's indeed a good thing to fix them regardless, but if the regressions are due the old-age of the OS (and considering the maybe Apple's end of support for it and the user base numbers), then IMO we could let some minor 10.6-only regressions be.

Handling regressions take real development resources, and we should strive to balance the value of fixing a regression with its cost.

We had a similar issue with creeping 10.6-only regressions at bug 985575, and I decided to let it go, but having a wider scale guidelines on this would be a good thing IMO.

Andreas?
Flags: needinfo?(gal)
I am only interested in a fix here because the same problem is likely to affect some mobile drivers as well (Vivante, I am looking at you here). If this was a pure 10.6-only problem, I would ask for talos to be turned off for 10.6 and this bug to be invalidated. I completely agree that performance on 10.6 is not essential for us. Mac has a high update cadence, unlike Windows.
Flags: needinfo?(gal)
Thanks for the good dialog in here.  My main goal is to document what regressions are taking place with the ideal scenario of them getting fixed, but in cases where we have an obscure test or platform at least document the changes so we have a clear paper trail.

I have filed bug 990490 to start a discussion about the usefulness of running talos tests on 10.6.
https://hg.mozilla.org/mozilla-central/rev/a63f5694f562
Assignee: nobody → bjacob
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.