Closed Bug 828126 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics: Layers, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: avih, Assigned: kats)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 820167, 826383
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
This bug prevents bugs 826383 and 820167 from being fully implemented, so they should be in the "blocks" list rather than the "depends" list.
Blocks: 826383, 820167
No longer depends on: 826383, 820167
Inbound is pretty seriously hosed today. New try push: https://tbpl.mozilla.org/?tree=Try&rev=27b93ae67a40
... 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.
Attached patch Remove android-specific ifdef (obsolete) — Splinter Review
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".
Assignee: avihpit → bugmail.mozilla
Attachment #701629 - Flags: review?(avihpit)
Attachment #701629 - Flags: review?(avihpit) → review+
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.
*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
Patching the gonk nsWindow didn't seem to do the trick: https://tbpl.mozilla.org/?tree=Try&rev=af96c2c847a7
Depends on: 830475
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/
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.
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.
Attachment #701629 - Attachment is obsolete: true
Attachment #702159 - Flags: review?(mwu)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.
Attachment #702159 - Flags: review?(mwu) → review?(avihpit)
Let's make sure this time ;)
https://tbpl.mozilla.org/?tree=Try&rev=21a34969449a
I guess I should have posted my own try push :)
https://tbpl.mozilla.org/?tree=Try&rev=3bdffca4993b
Attachment #702159 - Flags: review?(avihpit) → review+
https://hg.mozilla.org/mozilla-central/rev/c9c61f03c67f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.