805971 - Don't create and discard StartupCache instances on not-main processes

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

B2G makes heavy use of the plugin-container as we're pushing every app (or lots of apps) OOP. Running b2g-debug spams the console with lots of these warnings.

This warning is from StartupCache::Init() which is of course called after the instance has been created.

> [Child 687] WARNING: Startup cache is only available in the chrome process: file /home/tim/workspace/b2g-desktop/startupcache/StartupCache.cpp, line 147

Now ::Init() returns a failure and the instance is discarded again. The destructor produces the following output:

> [Child 687] WARNING: NS_ENSURE_TRUE(aFile) failed: file /home/tim/workspace/b2g-desktop/modules/libjar/zipwriter/src/nsZipWriter.cpp, line 232
> [Child 687] WARNING: could not open zipfile for write: file /home/tim/workspace/b2g-desktop/startupcache/StartupCache.cpp, line 426

I think we should probably not first create those instances just to discard them shortly after.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: simple patch

Don't know if we really need the warning?

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 675698 [details] [diff] [review]
simple patch

r=me with the warning put back. I'm a bit uncomfortable removing the warning without knowing who's trying to access the startup cache.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Thank you. I put the warning back in.

https://hg.mozilla.org/integration/mozilla-inbound/rev/25534b056ce0

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25534b056ce0

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 675698 [details] [diff] [review]
simple patch

I'm pretty sure we want this especially for B2G.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: There are lots of places that try to access the startup cache. If we fail earlier in OOP apps we can save time and memory by not creating and immediately discarding startup cache instances and their dependencies.
Testing completed (on m-c, etc.): Landed on m-c two days ago without problems.
Risk to taking this patch (and alternatives if risky): Small patch, low risk.
String or UUID changes made by this patch: None.

Comment 6

[Alex Keybl [:akeybl]]

(In reply to Tim Taubert [:ttaubert] from comment #5)
> User impact if declined: There are lots of places that try to access the
> startup cache. If we fail earlier in OOP apps we can save time and memory by
> not creating and immediately discarding startup cache instances and their
> dependencies.

Just because the reward here isn't totally explicit (seconds, percentages, etc.), can you suggest what the worst case scenario for desktop is? Or can you better justify the perf win here for B2G?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Alex Keybl [:akeybl] from comment #6)
> Just because the reward here isn't totally explicit (seconds, percentages,
> etc.), can you suggest what the worst case scenario for desktop is? Or can
> you better justify the perf win here for B2G?

I think there won't be much difference for desktop because we use separate processes only for plugins. B2G on the other hand might benefit from this because we'll need to allocate/free less memory on a device where memory is always low and where every app is ideally a separate process. I've not been able to measure any impact, the win here seems more theoretical :|

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> (In reply to Alex Keybl [:akeybl] from comment #6)
> > Just because the reward here isn't totally explicit (seconds, percentages,
> > etc.), can you suggest what the worst case scenario for desktop is? Or can
> > you better justify the perf win here for B2G?
> 
> I think there won't be much difference for desktop because we use separate
> processes only for plugins.

Who can we pull into this bug to confirm? Maybe bsmedberg? I just want to make sure the risk to desktop is totally manageable before taking a speculative fix in FF18.

Comment 9

[Benjamin Smedberg]

This should not be a risk to current desktop; we don't use "content" processes, just plugin processes which don't use the startupcache at all.

Although I'm a bit surprised: is there another bug about actually making content processes use the startup cache? Or will that not improve content process startup time?

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Although I'm a bit surprised: is there another bug about actually making
> content processes use the startup cache? Or will that not improve content
> process startup time?

That's a very good question - I don't have any idea about that, though. Not sure who's involved in maintaining the StartupCache, probably the perf team folks? (CC'ing some people.)

Comment 11

[Justin Lebar (not reading bugmail)]

> This should not be a risk to current desktop; we don't use "content" processes, just plugin 
> processes which don't use the startupcache at all.

If this is zero-risk for desktop, it sounds like the kind of thing we can take speculatively in B2G for the memory win and back out if it breaks something.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/19b9d31f75cc