Closed
Bug 1120149
Opened 9 years ago
Closed 9 years ago
Add hack to resolve AWSY regression in Gecko 36
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(2 files)
2.63 KB,
patch
|
tnikkel
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.35 KB,
patch
|
tnikkel
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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?
Assignee | ||
Comment 3•9 years ago
|
||
Try job here, based against current Aurora tip: https://tbpl.mozilla.org/?tree=Try&rev=6e7d4c06b47
Updated•9 years ago
|
Attachment #8547124 -
Flags: review?(tnikkel) → review+
Updated•9 years ago
|
Attachment #8547124 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
status-firefox36:
--- → affected
Assignee | ||
Comment 4•9 years ago
|
||
Thanks for the quick review and approval! Pushed to Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/1655198ac60a
Comment 5•9 years ago
|
||
I assume we're leaving this open until the AWSY regression is confirmed fixed. Setting the status flags now, though, for tracking purposes.
Assignee | ||
Comment 7•9 years ago
|
||
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.
status-firefox37:
wontfix → ---
Flags: needinfo?(seth)
Assignee | ||
Comment 8•9 years ago
|
||
I'm testing a fix that should work on 37 and 38 in this AWSY series: https://areweslimyet.com/?series=bug1100253_trunk
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
Here's a try job against trunk: https://tbpl.mozilla.org/?tree=Try&rev=b8446c4f4f31
Updated•9 years ago
|
Attachment #8552316 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Thanks for the review! This looks good to push; just waiting for the tree to reopen.
Assignee | ||
Comment 12•9 years ago
|
||
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/260ae5bafc64
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/260ae5bafc64
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox37:
--- → affected
status-firefox38:
--- → fixed
Updated•9 years ago
|
Attachment #8552316 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Target Milestone: mozilla36 → mozilla38
Assignee | ||
Comment 16•9 years ago
|
||
Not sure what the target milestone should be; a version of this patch is in 36, 37, and 38.
Comment 17•9 years ago
|
||
TM = when it landed on m-c :)
Comment 18•9 years ago
|
||
This appears to have had the desired effect on AWSY: ~40 MiB drops in the top four lines on the graph :)
Comment 19•9 years ago
|
||
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.
Description
•