Closed
Bug 988880
Opened 11 years ago
Closed 11 years ago
Consider turning off startup cache for b2g
Categories
(Core :: XPConnect, defect)
Tracking
()
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file)
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Taras probably has context on startup caching this stuff, in general.
Flags: needinfo?(taras.mozilla)
Thanks for looking at this!
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
(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.
Assignee | ||
Comment 6•11 years ago
|
||
(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)
Comment 7•11 years ago
|
||
> Does the startup cache consume any memory?
Yes.
Comment 8•11 years ago
|
||
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.
Updated•11 years ago
|
Flags: needinfo?(aklotz)
Assignee | ||
Comment 9•11 years ago
|
||
(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)
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
(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
Comment 12•11 years ago
|
||
I feel like somebody would have already noticed if b2g was using 7mb for a startup cache...
Comment 13•11 years ago
|
||
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.
Assignee | ||
Comment 14•11 years ago
|
||
(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.
Sure, let's do it.
Flags: needinfo?(khuey)
Assignee | ||
Comment 16•11 years ago
|
||
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)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8409549 -
Flags: review?(fabrice)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → bobbyholley
Assignee | ||
Comment 18•11 years ago
|
||
Updated•11 years ago
|
Attachment #8409549 -
Flags: review?(fabrice) → review+
Updated•11 years ago
|
blocking-b2g: --- → 1.3T+
Flags: needinfo?(fabrice)
Assignee | ||
Comment 19•11 years ago
|
||
Comment 20•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 21•11 years ago
|
||
status-b2g-v1.3T:
--- → fixed
Comment 22•11 years ago
|
||
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?
Assignee | ||
Comment 23•11 years ago
|
||
(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)
Comment 24•11 years ago
|
||
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)
Comment 25•11 years ago
|
||
FTR, AutomatedTester is looking into this and will hand off to mdas tomorrow am as needed.
Comment 26•11 years ago
|
||
FYI, this doesn't reproduce on a desktop Firefox build with the patch adjusted accordingly. Trying now on b2g desktop...
Comment 27•11 years ago
|
||
This does reproduce on a B2G desktop build.
Comment 28•11 years ago
|
||
The error is coming from here: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/ChromePowers.js#20
Comment 29•11 years ago
|
||
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.
Assignee | ||
Comment 30•11 years ago
|
||
(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)
Comment 32•11 years ago
|
||
Assignee | ||
Comment 33•11 years ago
|
||
Thanks for pushing that through jgriffin!
Comment 34•11 years ago
|
||
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.
Description
•