Kris Maglione [:kmag] Assignee
	Description • 6 years ago

We initially started eagerly encoding cached scripts due to some flakiness we were seeing in crash reports, that we couldn't figure out the source of. That's not a big deal most of the time, but in the child process, we never wind up freeing the encoded data unless we send it back to the parent process, which we only ever do for the first content process.

In general, this should only be an issue when the startup cache is flushed, or we start loading new scripts in content processes for some other reason, but it still makes me uncomfortable (and could add up to a lot in sessions that need a cache flush).

	Eric Rahm [:erahm]
	Comment 2 • 6 years ago
mozreview-review

Comment on attachment 8987402 [details]
Bug 1470793: Stop eagerly XDR encoding scripts in the preloader cache.

https://reviewboard.mozilla.org/r/252650/#review259838

::: js/xpconnect/loader/ScriptPreloader.cpp
(Diff revision 1)
> -    // exist in the child cache, encode it now, before it's ever executed.
> -    //
> -    // Ideally, we would like to do the encoding lazily, during idle slices.
> -    // There are subtle issues with encoding scripts which have already been
> -    // executed, though, which makes that somewhat risky. So until that
> -    // situation is improved, and thoroughly tested, we need to encode eagerly.

I guess we need to keep any eye out for new crashes then?

	Kris Maglione [:kmag] Assignee
	Comment 3 • 6 years ago

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #2)
> Comment on attachment 8987402 [details]
> Bug 1470793: Stop eagerly XDR encoding scripts in the preloader cache.
> 
> https://reviewboard.mozilla.org/r/252650/#review259838
> 
> ::: js/xpconnect/loader/ScriptPreloader.cpp
> (Diff revision 1)
> > -    // exist in the child cache, encode it now, before it's ever executed.
> > -    //
> > -    // Ideally, we would like to do the encoding lazily, during idle slices.
> > -    // There are subtle issues with encoding scripts which have already been
> > -    // executed, though, which makes that somewhat risky. So until that
> > -    // situation is improved, and thoroughly tested, we need to encode eagerly.
> 
> I guess we need to keep any eye out for new crashes then?

Ted and nbp are already keeping track of XDR crashes. This change didn't seem to have any effect on them, though, so I'm not super worried. It was mainly just defensive until we figured things out.

	Kris Maglione [:kmag] Assignee
	Comment 4 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/841ed9c78bf765e7d9f40614c29853948b6680d0
Bug 1470793: Stop eagerly XDR encoding scripts in the preloader cache. r=erahm

	Cosmin Sabou [:CosminS]
	Comment 5 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/841ed9c78bf7

	Ionuț Goldan [:igoldan]
	Comment 6 • 6 years ago

Seems like this won us some perf:

== Change summary for alert #14073 (as of Wed, 27 Jun 2018 16:21:09 GMT) ==

Improvements:

  3%  cpstartup content-process-startup linux64-qr opt e10s stylo     203.42 -> 196.92
  3%  cpstartup content-process-startup linux64 opt e10s stylo        189.92 -> 184.25
  2%  cpstartup content-process-startup linux64 pgo e10s stylo        183.17 -> 178.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14073

	Ionuț Goldan [:igoldan]
	Comment 7 • 6 years ago

Plus some AWSY gains! Both these improvements are either related to this bug or bug 1471089.

== Change summary for alert #14064 (as of Wed, 27 Jun 2018 16:21:09 GMT) ==

Improvements:

 16%  Base Content Explicit windows7-32 pgo stylo     11,811,498.67 -> 9,922,218.67
 14%  Base Content Explicit windows10-64 pgo stylo    14,783,658.67 -> 12,769,280.00
 12%  Base Content Explicit windows10-64 opt stylo    14,468,949.33 -> 12,709,546.67
 12%  Base Content Explicit linux64 opt stylo         18,395,648.00 -> 16,167,936.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14064