Closed Bug 856807 Opened 8 years ago Closed 5 years ago

Talos Regression tsvg_nochrome 6.4% on Android March 29


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: gbrown, Unassigned)



(Whiteboard: [leave open])


(1 file)

Reported in dev-tree-management Digest, Vol 52, Issue 2:

Message: 1
Date: Mon, 01 Apr 2013 18:41:44 -0000
Subject: <Regression> mobile - SVG NoChrome - Android 4.0.4 - 6.45%
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   :

Changeset range:
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. is basically 8b386ed03fb5 (part 3.5) and does not seem to show the regression: tsvg_nochrome: 4502.27

Pushing with revision d9916e4fb4eb.
That doesn't seem to be it either: tsvg_nochrome: 4438.73

Pushing with revision 643b78450848.
tsvg_nochrome: 4726.91

So revision 643b78450848 looks like the culprit...
I'm doing some extra Talos runs to make sure.
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.
(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):

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:
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: 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.

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. is a patch that basically adds the SetClipRect(nullptr) back, on near-current inbound. Let's see if it reverses the regression. is the near-current inbound that patch is based on, for baseline performance purposes.
Yes, so that silly patch does fix the regression.
Attached patch super hackSplinter Review
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+
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.
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.