1120149 - Add hack to resolve AWSY regression in Gecko 36

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Add a hack to resolve an AWSY regression in Gecko 36

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Try job here, based against current Aurora tip:

https://tbpl.mozilla.org/?tree=Try&rev=6e7d4c06b47

Comment 4

[Seth Fowler [:seth] [:s2h]]

Thanks for the quick review and approval! Pushed to Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/1655198ac60a

Comment 5

[Ryan VanderMeulen [:RyanVM]]

I assume we're leaving this open until the AWSY regression is confirmed fixed. Setting the status flags now, though, for tracking purposes.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Are we good here? :)

Comment 7

[Seth Fowler [:seth] [:s2h]]

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.

Comment 8

[Seth Fowler [:seth] [:s2h]]

I'm testing a fix that should work on 37 and 38 in this AWSY series:

https://areweslimyet.com/?series=bug1100253_trunk

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: Add a hack to resolve an AWSY regression in Gecko 37 and 38

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.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Here's a try job against trunk:

https://tbpl.mozilla.org/?tree=Try&rev=b8446c4f4f31

Comment 11

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! This looks good to push; just waiting for the tree to reopen.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/260ae5bafc64

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/260ae5bafc64

Comment 14

[Seth Fowler [:seth] [:s2h]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c1be585ca75

Comment 16

[Seth Fowler [:seth] [:s2h]]

Not sure what the target milestone should be; a version of this patch is in 36, 37, and 38.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

TM = when it landed on m-c :)

Comment 18

[Nicholas Nethercote [inactive]]

This appears to have had the desired effect on AWSY: ~40 MiB drops in the top four lines on the graph :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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