Closed Bug 988880 Opened 6 years ago Closed 6 years ago

Consider turning off startup cache for b2g

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- ?
b2g-v2.0 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

I was just poking around at some code, and found this:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSLoaderUtils.cpp?rev=2d2709188afa#37

Looks like Kyle added this when he merged all the system compartments on b2g (bug 798491). That case calls {Read,Write}CachedFunction (which is unimplemented per the above) instead of {Read,Write}CachedScript.

Is startup caching this stuff a win? If so, it seems like we should turn it on for b2g. If not, we should rip it out for desktop too.
Taras probably has context on startup caching this stuff, in general.
Flags: needinfo?(taras.mozilla)
Bobby,
see https://bugzilla.mozilla.org/show_bug.cgi?id=873638#c1

Startup cache is a win on warm startup(probly almost never on b2g due to limited ram?). It's a loss on cold startup due to requiring more IO(which might be due to being stored in utf16). 

Aaron can help you figure out how to benchmark this for mobile where disk/cpu tradeoffs should be somewhat different.
Flags: needinfo?(taras.mozilla) → needinfo?(aklotz)
(In reply to Taras Glek (:taras) from comment #3)
> Bobby,
> see https://bugzilla.mozilla.org/show_bug.cgi?id=873638#c1
> 
> Startup cache is a win on warm startup(probly almost never on b2g due to
> limited ram?). It's a loss on cold startup due to requiring more IO(which
> might be due to being stored in utf16).

Hm. I guess with Nuwa, we basically only do startup once (at boot time), right? How much do we care about boot time, and how significant would this be?
App-start post initial start is much more important than initial start.
(In reply to Jonas Sicking (:sicking) from comment #5)
> App-start post initial start is much more important than initial start.

And with Nuwa, the startup cache basically doesn't help us at all there, right?

Does the startup cache consume any memory? Maybe we should disable it entirely on b2g.
Flags: needinfo?(khuey)
> Does the startup cache consume any memory?

Yes.
Depends on: 989373
The test from https://bugzilla.mozilla.org/show_bug.cgi?id=873638#c1 was essentially a measurement of elapsed time between process start and sessionstore-windows-restored.

For the NoStartupCache test case I used a custom build with changes to mozJSComponentLoader.cpp and mozJSSubScriptLoader to remove startup cache accesses from those code paths. I can dig up those patches if they would help.
Flags: needinfo?(aklotz)
(In reply to Nicholas Nethercote [:njn] from comment #7)
> > Does the startup cache consume any memory?

How much? Seems like we should straight-up disable it on b2g given Nuwa.
Flags: needinfo?(n.nethercote)
There are entries in about:memory: startup-cache/mapping and startup-cache/data. Typically they combine to use between 2 and 7 MiB on desktop. I don't know how much they use on B2G.
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #10)
> There are entries in about:memory: startup-cache/mapping and
> startup-cache/data. Typically they combine to use between 2 and 7 MiB on
> desktop. I don't know how much they use on B2G.

Yikes. Morphing this bug. khuey, can you weigh in?
No longer depends on: 989373
Summary: Startup cache unimplemented for JS Components on b2g → Consider turning off startup cache for b2g
I feel like somebody would have already noticed if b2g was using 7mb for a startup cache...
To be more concrete, in this about:memory report I have after running some test on b2g for 9 hours, startup-cache is using 0.22mb. (for one process)  Which isn't nothing, but I don't know if that's enough to consider removing and investigating perf problems etc.
(In reply to Andrew McCreight [:mccr8] from comment #13)
> To be more concrete, in this about:memory report I have after running some
> test on b2g for 9 hours, startup-cache is using 0.22mb. (for one process) 
> Which isn't nothing, but I don't know if that's enough to consider removing
> and investigating perf problems etc.

Turning off the startup cache is very easy IIUC, so if it really isn't buying us anything, we might as well save .22 mb.
Blocks: 989373
Sure, let's do it.
Flags: needinfo?(khuey)
Fabrice, this would be a very simple patch, and save .22 MiB on b2g. Do we want to make this happen for tarako, or is it too late?
Flags: needinfo?(fabrice)
Attachment #8409549 - Flags: review?(fabrice)
Assignee: nobody → bobbyholley
Attachment #8409549 - Flags: review?(fabrice) → review+
blocking-b2g: --- → 1.3T+
Flags: needinfo?(fabrice)
https://hg.mozilla.org/mozilla-central/rev/87a0bf86dc09
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Backed out from 1.3t for Mnw failures.
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/bba416e2e377

https://tbpl.mozilla.org/php/getParsedLog.php?id=38332902&tree=Mozilla-B2g28-v1.3t

On a related topic, should we consider this for v1.4 as well?
status-b2g-v1.4: --- → ?
Flags: needinfo?(bobbyholley)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22) 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38332902&tree=Mozilla-B2g28-
> v1.3t

This looks like a relatively simple failure, and doesn't appear on trunk. So I think someone with a better understanding of the marionette harness should have a look here. jgriffin, do you think you could get someone to take a quick look at this right away? We need to get this sorted out by friday if we want this memory win on Tarako.

> On a related topic, should we consider this for v1.4 as well?

Potentially, sure.
Flags: needinfo?(bobbyholley) → needinfo?(jgriffin)
It's this block of code which is apparently failing in these tests, but I don't know why:

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#673

If that's not enough to go on, please ping mdas or myself on #ateam.
Flags: needinfo?(jgriffin)
FTR, AutomatedTester is looking into this and will hand off to mdas tomorrow am as needed.
FYI, this doesn't reproduce on a desktop Firefox build with the patch adjusted accordingly.  Trying now on b2g desktop...
This does reproduce on a B2G desktop build.
Ok, have a fix.  This goes away if applying this change:

https://hg.mozilla.org/mozilla-central/diff/fbeed56db621/testing/specialpowers/content/specialpowersAPI.js

Alternately, you could uplift the entire push that change comes from, bug 682048, but it doesn't apply 100% cleanly.
(In reply to Jonathan Griffin (:jgriffin) from comment #29)
> Ok, have a fix.  This goes away if applying this change:
> 
> https://hg.mozilla.org/mozilla-central/diff/fbeed56db621/testing/
> specialpowers/content/specialpowersAPI.js

Cool!

Ryan, if you have a moment when you wake up, could you reland the patch to tarako along with the specialpowersAPI.js change above?
Flags: needinfo?(ryanvm)
I can do this.
Flags: needinfo?(ryanvm)
Thanks for pushing that through jgriffin!
Thanks Jonathan!!  FWIW <https://hg.mozilla.org/mozilla-central/diff/fbeed56db621/testing/specialpowers/content/specialpowersAPI.js> being necessary totally makes sense, without that we wouldn't export these names.
You need to log in before you can comment on or make changes to this bug.