Closed Bug 1120149 Opened 5 years ago Closed 5 years ago

Add hack to resolve AWSY regression in Gecko 36

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files)

Bug 1100253 is a ~40MB AWSY regression that was caused by refactoring in ImageLib and landed in Gecko 36.

I have tried, but so far failed, to figure out the exact cause of the regression. The change in ImageLib that triggered it simply stopped sending onload block/unblock notifications for redecodes of images. Some code somewhere has a bug that causes it to depend on receiving extra block/unblock onload notifications. Because the problem is intermittent, even on AWSY, and nobody is able to reproduce it locally, it's very hard to debug.

I have determined that adding a hack in ProgressTracker to send an extra block/unblock onload pair every time an image is redecoded fixes the regression. Given that 36 is on Aurora now and is going to merge to Beta very soon, I think it's worth pushing this hack to Aurora (and probably creating a version of the fix for trunk) until we can figure out the real cause of the bug.
Here's the hack. This has the effect of having us send BlockOnload and
UnblockOnload every time ProgressTracker sees FLAG_DECODE_COMPLETE, even if
they've already been sent.
Attachment #8547124 - Flags: review?(tnikkel)
Comment on attachment 8547124 [details] [diff] [review]
Add a hack to resolve an AWSY regression in Gecko 36

Approval Request Comment
[Feature/regressing bug #]: 1084136
[User impact if declined]: Possible serious regression in memory usage. (~40MB regression on AWSY.)
[Describe test coverage new/current, TBPL]: This patch cannot land anywhere but Gecko 36. It has been tested on try.
[Risks and why]: Fairly low risk. It should be safe to send extra onload blocking notifications, which is what this patch does.
[String/UUID change made/needed]: None.
Attachment #8547124 - Flags: approval-mozilla-aurora?
Try job here, based against current Aurora tip:

https://tbpl.mozilla.org/?tree=Try&rev=6e7d4c06b47
Attachment #8547124 - Flags: review?(tnikkel) → review+
Attachment #8547124 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks for the quick review and approval! Pushed to Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/1655198ac60a
I assume we're leaving this open until the AWSY regression is confirmed fixed. Setting the status flags now, though, for tracking purposes.
Target Milestone: --- → mozilla36
Are we good here? :)
Flags: needinfo?(seth)
So we *will* fix this on 37. I just need to write a version of the patch that works on 37/38, which I'm doing right now.
Flags: needinfo?(seth)
I'm testing a fix that should work on 37 and 38 in this AWSY series:

https://areweslimyet.com/?series=bug1100253_trunk
OK, it looks like a straightforward port of the hack from Gecko 36 solves the
regression on Gecko 38 (and presumably thus also 37). Here's the patch.
Attachment #8552316 - Flags: review?(tnikkel)
Here's a try job against trunk:

https://tbpl.mozilla.org/?tree=Try&rev=b8446c4f4f31
Attachment #8552316 - Flags: review?(tnikkel) → review+
Thanks for the review! This looks good to push; just waiting for the tree to reopen.
https://hg.mozilla.org/mozilla-central/rev/260ae5bafc64
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8552316 [details] [diff] [review]
Add a hack to resolve an AWSY regression in Gecko 37 and 38

Approval Request Comment
[Feature/regressing bug #]: 1084136
[User impact if declined]: 40MB increase in memory usage on AWSY benchmark.
[Describe test coverage new/current, TreeHerder]: A variant of this patch has been on Beta for some time, and this is already on central.
[Risks and why]: Low risk. We're doing the same thing on Beta with no issues.
[String/UUID change made/needed]: None.
Attachment #8552316 - Flags: approval-mozilla-aurora?
Attachment #8552316 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone: mozilla36 → mozilla38
Not sure what the target milestone should be; a version of this patch is in 36, 37, and 38.
TM = when it landed on m-c :)
This appears to have had the desired effect on AWSY: ~40 MiB drops in the top four lines on the graph :)
This was causing conflicts with the beta uplift for bug 1112956 and bug 1112972. To get around them, I backed out the original landing and re-landed the 37/38 patch.

https://hg.mozilla.org/releases/mozilla-beta/rev/914dfaa20eef
https://hg.mozilla.org/releases/mozilla-beta/rev/7422906b1a32
You need to log in before you can comment on or make changes to this bug.