1260894 - The LazyScriptCache is secretly disabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

See bug 988353.

Since the cache was added in bug 883154, we've added the ability to lazy-parse functions that use variables from enclosing scopes. So we can't just land the patch in bug 988353 and re-enable the cache - the hashing scheme is longer correct, and it looks like we've made GC changes that affect this too:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5edb536095b9&selectedJob=18825828

But we shouldn't just leave it - it's really misleading having the code there when it's not doing anything.

Comment 1

[Jason Orendorff [:jorendorff]]

We could remove it, or we could fix it. I guess it depends on how this optimization fits in with future work. ni?nbp and ni?bhackett to figure out what makes sense here.

Comment 2

[Nicolas B. Pierron [:nbp]]

This LazyScriptCache is orthogonal to the XDRLazyScript encoding/decoding function.
Removing or fixing this feature should have no impact on the future bytecode cache (Bug 900784 & Bug 917996).

Bug 883154 comment 0 highlights an intent similar to what would be achieved by the bytecode cache, except that the bytecode cache only aims at skipping the initial parse-time, while the LazyScriptCache aims at skipping the parse-time of the innermost LazyScript.

The bytecode cache aims to save the JSScript used during the start-up and the Lazyscript of unused functions on the disk, and use XDR decoding instead of the parser.

Comment 3

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a1af65ec737

Comment 4

[Brian Hackett [Laid off!]]

Well, since we now consolidate script sources with the same text it should be easier to get the lazy script cache to behave correctly now, but I won't be working on it in the foreseeable future so it is probably best to just remove it.

Comment 5

[Jason Orendorff [:jorendorff]]

Actually that makes this pretty attractive, but I'll let nbp decide. I can either #ifdef out the code, and we can decide what to do with it later; or just delete it.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Number calls to CompileLazyFunction in functions of the load-time.

This graph is produced by browsing ~15 facebook pages, and ~5 twitter pages in different tabs.

X axes:
  Time since the source file got loaded.

Y axes:
  Number calls to CompileLazyFunction at the given time. (100ms slices)

This graph has 2 curves:
 - the purple one, without LazyScriptCache.
 - the green one, with an infinite LazyScriptCache and without relazification.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Number of remaining calls to CompileLazyFunction in function of the Bytecode cache.

This graph is produced by browsing ~15 facebook pages, and ~5 twitter pages in different tabs.

X axes:
  Time slice captured by the bytecode cache.

Y axes:
  Number of remaining calls to CompileLazyFunction. (until the first 20s browsing)

This graph has 2 curves:
 - the purple one, without LazyScriptCache.
 - the green one, with an infinite LazyScriptCache and without relazification.

Comment 8

[Nicolas B. Pierron [:nbp]]

The previous data seems to be in favor of the LazyScriptCache, still I am not yet convinced by the current data, as these data are computed assuming that we have an infinite LazyScriptCache and without relazification.

I do not think the LazyScriptCache is likely to have a similar amount of hits without this hypothesis.

The reason I do not think this is going to hold, is because we are going to relazify either at the end of the execution of a script execution (maybeGC), or because weran a shrinking GC caused by leaving the browser idle for a few minutes.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Equivalent time spent/saved under CompileLazyFunction.

This graph is produced by browsing ~15 facebook pages, and ~5 twitter pages in different tabs.

These numbers are made while simulating the bytecode cache, and the LazyScriptCache, while having a GC every 5 minutes which discard the JSScript for the function.

Note, the experiment is quite in favor of the LazyScript Cache, but the forced GC is not realistic either, as this does not consider scripts which are still needed for the execution.

X axes:
  Time slice captured by the bytecode cache.

Y axes:
  Equivalent time (in ms) spent/saved under CompileLazyFunction.


This graph has 3 stacked non-overlapping surfaced:
 - the purple one, is the time saved by the LazyScriptCache.
 - the green one, is the time saved by the bytecode cache.
 - the blue one, is the time spent under CompileLazyFunction.

Conclusion:

Even if this graph does not consider all the variables, I think it highlights the fact that the bytecode cache and the LazyScript cache are orthogonal.  The ratio of LazyScript cache hits compared to the remaining CompileLazyfunction hits is roughtly the same (~40% for the current experiment).

The bytecode cache will reduce the initial time spent under CompileLazyFunction, which saves time spent in CompileLazyFunction, and removes cache hits of the LazyScript cache.

The bytecode cache will save the load latency, while the LazyScript cache will save the throughput during the execution of the page.

Even if I think this is nice throughput saving (1/6 of the CompileLazyFunction time in this experiment).  I am not going to be working on it in the next 3 months, and I have no idea yet for the future.

So, if we don't find anybody interested in taking over this work, I guess we should consider removing it.

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: Bug 1260894 - Remove broken LazyScriptCache

Due to a shadowed variable in FixedSizeHash, the LazyScriptCache has not
been successfully adding items to the cache. This has been broken for a
while now and the lookup key and other constraints are no longer valid.
This patch removes the cache altogether until someone can re-qualify the
correctness and utility.

Review commit: https://reviewboard.mozilla.org/r/198116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198116/

Comment 11

[Ted Campbell [:tcampbell]]

Here is a patch to remove the cache, the hashing functions, and the broken FixedSizeHash altogether. The effort to re-qualify the cache behavior is far more than digging this code out of hg so let's remove it.

If the cache is re-enable in current code, we have a sea of orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2d68c485968fd03f483ef8f8c320385d85dd19f

We can create a new bug to track re-implementing this. There are concerns around GC as well as various compile flags such as runOnce, non-syntactic globals, etc that need to be handled in a new impl.

Comment 12

[Ted Campbell [:tcampbell]]

Function qualifiers like async are also broken in the current key function.

Comment 13

[Ted Campbell [:tcampbell]]

Also, the memcmp in the match function is given a length in uint16_t chars instead of in bytes. yikes

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 8926873 [details]
Bug 1260894 - Remove broken LazyScriptCache

https://reviewboard.mozilla.org/r/198116/#review203450

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0275d025fdb4
Remove broken LazyScriptCache r=jorendorff

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/0275d025fdb4