Closed Bug 856807 Opened 8 years ago Closed 5 years ago
Talos Regression tsvg
_nochrome 6 .4% on Android March 29
Reported in dev-tree-management Digest, Vol 52, Issue 2: Message: 1 Date: Mon, 01 Apr 2013 18:41:44 -0000 From: firstname.lastname@example.org To: email@example.com Subject: <Regression> mobile - SVG NoChrome - Android 4.0.4 - 6.45% Message-ID: <20130401184144.D57D1104125@cruncher.srv.releng.scl3.mozilla.com> Content-Type: text/plain; charset="us-ascii" Regression: mobile - SVG NoChrome - Android 4.0.4 - 6.45% increase ------------------------------------------------------------------ Previous: avg 4447.058 stddev 55.993 of 30 runs up to revision e4d8e21d0788 New : avg 4733.726 stddev 42.229 of 5 runs since revision 1932c6f78248 Change : +286.668 (6.45% / z=5.120) Graph : http://mzl.la/X9324s Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e4d8e21d0788&tochange=1932c6f78248
It looks to me like this was caused by bug 852489: m-i rev ad75012bfabf (bug 852489): tsvg_nochrome: 4652.55 Previous m-i rev 5a7aaa967ad3: tsvg_nochrome: 4478.45
Odd. I'll do some try pushes to see if I can narrow it down. It's possible it's 8b386ed03fb5, which fixed real bugs where we didn't call Mutated() when we should but would have added a little overhead. https://tbpl.mozilla.org/?tree=Try&rev=92941f87b306 https://tbpl.mozilla.org/?tree=Try&rev=fd051d24e36e
https://tbpl.mozilla.org/?tree=Try&rev=fd051d24e36e is basically 8b386ed03fb5 (part 3.5) and does not seem to show the regression: tsvg_nochrome: 4502.27 Pushing https://tbpl.mozilla.org/?tree=Try&rev=3ef214e9e546 with revision d9916e4fb4eb.
That doesn't seem to be it either: tsvg_nochrome: 4438.73 Pushing https://tbpl.mozilla.org/?tree=Try&rev=3000950b868d with revision 643b78450848.
tsvg_nochrome: 4726.91 So revision 643b78450848 looks like the culprit...
I'm doing some extra Talos runs to make sure. https://hg.mozilla.org/try/rev/643b78450848 I don't understand this at all. As far as I can tell, this is a simple patch that replaces some code with equivalent code that's just the same or faster. Matt, do you have any ideas?
My only guess is the SetClipRect(nullptr) + IntersectClipRect() -> SetClipRect() change. That was just two sets of the clip rect, and now we're adding in an IsEqualEdges check each time (though probably skipping the Mutated()) call. For a massive number of inactive layers (like svg tends to do), where Mutated() is a no-op, this could add up to something.. I guess?
Maybe skip checking IsEqualEdges and just overwrite the value if Mutated isn't going to do anything?
I find that unbelievable, but I guess I'll give it a go. I can also break this patch up into smaller pieces.
First half of part 6 (ColorLayers and ImageLayers): https://tbpl.mozilla.org/?tree=Try&rev=4681c61f98c2
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9) > I find that unbelievable, but I guess I'll give it a go. I can also break > this patch up into smaller pieces. I do too, but I can't see anything else that has an actual functional change apart from that.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > First half of part 6 (ColorLayers and ImageLayers): > https://tbpl.mozilla.org/?tree=Try&rev=4681c61f98c2 No regression.
You should try the ThebesLayer part next, but leave the SetClipRect(nullptr) call in CreateOrRecycleThebesLayer. I suspect no regression with that either.
Yeah, no regression. But doesn't that contradict your theory in comment #8? Since we are doing those IsEqualEdges checks now. Re-pushing the final part of part 6 to make sure I'm not losing my mind: https://tbpl.mozilla.org/?tree=Try&rev=2701623f8a60
I don't think so? With the patch from comment 14, we're calling SetClipRect twice for every thebes layer. Once with nullptr (clearing the previous clip rect), and once with &emptyRect (which should just set the rect without an IsEqualEdges check since mHasClipRect will be false). Once we drop the former SetClipRect call, then the latter will happen with mHasClipRect true, and we'll have to run IsEqualEdges.
oh right. But it's still insane that IsEqualEdges would take any time at all.
OK, that push reproduced the regression. This is daft. Made another try push with a variation on that patch: https://tbpl.mozilla.org/?tree=Try&rev=ac0e439294f5 https://hg.mozilla.org/try/rev/8ff16c9787a2 moves SetClipRect(nullptr) from CreateOrRecycleThebesLayer down to just before thebesLayer->SetClipRect(&emptyRect) (and also in the parallel code path).
That change shows no regression, as expected I guess.
https://tbpl.mozilla.org/?tree=Try&rev=53f95f0e62a1 I ended up adding an mHasShadow bool to Layer, and I'm trying to use this to avoid IsEqualEdges. But this whole thing is just nuts.
It looks like that attempt still causes a regression.
https://tbpl.mozilla.org/?tree=Try&rev=b6020b334d6e is a patch that basically adds the SetClipRect(nullptr) back, on near-current inbound. Let's see if it reverses the regression. https://tbpl.mozilla.org/?tree=Try&rev=58d979ea061c is the near-current inbound that patch is based on, for baseline performance purposes.
Yes, so that silly patch does fix the regression.
I suggest we land this patch for now but leave the bug open. I don't have time to find the real issue. Maybe it will be easier to figure out when we have OMTC on more platforms.
Assignee: nobody → roc
Attachment #739379 - Flags: review?(matt.woodrow)
Attachment #739379 - Flags: review?(matt.woodrow) → review+
Whiteboard: [leave open]
Backed out for causing intermittent Linux32 reftest-ipc timeouts (~30% of the time). https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d35eccad01 https://tbpl.mozilla.org/php/getParsedLog.php?id=22014478&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=22014385&tree=Mozilla-Inbound
The patch doesn't make sense and I don't want to try to debug this timeout issue. If turning on OMTC regresses tsvg_nochrome on Mac then let's debug it there, otherwise, I'm going to ignore this.
can we mark this as wontfix? seems to be old and the browser has changed significantly, let alone no simple way to test this.
Assignee: roc → nobody
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.