990233 - 5.16% tresize regression on osx 10.6 seen on inbound

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

found on dev.tree-management:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/h6hIp33Hvs0

we have a 5% regression on tresize, here is the graph:
http://graphs.mozilla.org/graph.html#tests=[[254,63,21]]&sel=1395691933702,1396296733702&displayrange=7&datatype=running

here is a reduced view of tbpl (I did some retriggers):
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=5636e18a27f5&tochange=72ba89a774db&jobname=Rev4%20MacOSX%20Snow%20Leopard%2010.6%20mozilla-inbound%20talos%20chromez

This is information about tresize:
https://wiki.mozilla.org/Buildbot/Talos/Tests#tresize

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 2

[Andreas Gal :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.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 4

[Andreas Gal :gal]

Actually hold that thought. 10.6 only regression I am ok with carrying actually. What do you think?

Comment 5

[Andreas Gal :gal]

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.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 7

[Andreas Gal :gal]

No worries, backing out is never wrong :) We will just reland.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 9

[Avi Halachmi (:avih)]

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?

Comment 10

[Andreas Gal :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.

Comment 11

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

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a63f5694f562