828126 - Remove #ifdef ANDROID in bug 820167 (possibly also bug 826383)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Avi Halachmi (:avih)]

To remove the dependency on bug 785597, #ifdef ANDROID were landed in bug 820167 (possibly also bug 826383).

This bug is to track the android crash bug and remove those ifdef when possible.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I've landed the patches from bug 826300 and bug 785597 to fix the underlying issues (*fingers crossed*).

Pushed a patch to try that removes the ifdefs from bug 820167: https://tbpl.mozilla.org/?tree=Try&rev=7bc40f82309c

Comment 2

[Kartikaya Gupta (email:[e-mail])]

This bug prevents bugs 826383 and 820167 from being fully implemented, so they should be in the "blocks" list rather than the "depends" list.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

This time based on a non-broken cset: https://tbpl.mozilla.org/?tree=Try&rev=6eb6bee2f74c

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Inbound is pretty seriously hosed today. New try push: https://tbpl.mozilla.org/?tree=Try&rev=27b93ae67a40

Comment 5

[Kartikaya Gupta (email:[e-mail])]

... and once more, this time due to ftp issues. https://tbpl.mozilla.org/?tree=Try&rev=564091a7798b - this one shows that rck2 is failing but only on the pandas. Not sure why.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Remove android-specific ifdef

Actually rck2 is already failing on the pandas, and it's hidden by default on m-c and m-i. So my patch is actually "green".

Comment 7

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecac2ce4eada

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0d73ea9f22 - the b2g emulator tests are usually pretty opaque about their failures, but I'm guessing the timeout in startup on every test means... it timed out in startup.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

*sigh*. I suspect B2G has exactly the same problem that we did on android, which is that its GetLayerManager gets called on two different nsWindow instances, and it goes and creates two compositors, which then blows up. I could apply the same patch I did for android, but I'm not sure if having multiple nsWindows is allowed in B2G, and if this case needs to be handled differently.

Alternatively I could just replace the #ifdef ANDROID with #ifdef GONK and let the B2G guys handle it on their end :p

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Patching the gonk nsWindow didn't seem to do the trick: https://tbpl.mozilla.org/?tree=Try&rev=af96c2c847a7

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Yikes. This patch also caused a 0.5meg regression in explicit memory usage and a 2.5 meg regression in resident memory usage which disappeared when it was backed out. Click on the "Start" line data points for Jan 13 here: http://people.mozilla.com/~kgupta/awsy/

Comment 12

[Avi Halachmi (:avih)]

By default (currently), removing the #ifdef's should result in zero memory increase. The call which the ifdef masks only changes a member (timestamp) in LayerManager.

When the system is actually used to record frames (bug 828097 will add tabs animation telemetry probes at the desktop browser - which use frames recording), it will allocate 4.5kb of buffers.

My guess is that requesting LayerManager too early results in the excess allocation, regardless if the call itself is actually being issued.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Turn android-specific ifdef to a gonk-specific ifdef

Modify the ifdef condition. The code works fine in Fennec, and I'd like to enable to catch future regressions in it. I have filed bug 830475 for fixing the Gonk codepath.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Yikes. This patch also caused a 0.5meg regression in explicit memory usage
> and a 2.5 meg regression in resident memory usage which disappeared when it
> was backed out. Click on the "Start" line data points for Jan 13 here:
> http://people.mozilla.com/~kgupta/awsy/

Turns out this was caused by loading the graphics stack into memory sooner. The bulk of the increased memory usage was attributed to libGLESv2_adreno200.so and friends. I also note on the awsy graph at http://people.mozilla.com/~kgupta/awsy/#which=StartSettled,when=37 the value at "StartSettled" doesn't change significantly, which means that the steady-state memory usage is still the same; it's just the graphics stuff gets loaded a little earlier and so shows up in the "Start" line as well.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 702159 [details] [diff] [review]
Turn android-specific ifdef to a gonk-specific ifdef

Changing reviewer after discussion with vladan since mwu hasn't gotten back to me and the change should be straightforward enough. I can re-push to try to ensure it hasn't broken since I wrote it.

Comment 16

[Avi Halachmi (:avih)]

Let's make sure this time ;)
https://tbpl.mozilla.org/?tree=Try&rev=21a34969449a

Comment 17

[Kartikaya Gupta (email:[e-mail])]

I guess I should have posted my own try push :)
https://tbpl.mozilla.org/?tree=Try&rev=3bdffca4993b

Comment 18

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c61f03c67f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c9c61f03c67f