1020012 - Consolidate ScriptSources with the same source

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

When we parse the same source file multiple times we can end up with multiple ScriptSource objects with the same underlying text.  While we (usually) compress this text the memory consumed by the compressed text still adds up.  Additionally, since we don't keep track of which ScriptSources are the same it is hard to easily compare scripts loaded at different times, which we'd like to do for the lazy script cache.

The attached patch tries to improve this situation by combining ScriptSources with the same text.  This adds a hashtable to JSRuntime keeping track of all the compressed ScriptSources in existence.  When we try to add a compressed ScriptSource to the table, if there is already one with the same compressed source then the new one is changed to free its compressed data and defer source requests to the ScriptSource already in the table.

While testing I see a lot of duplicate scripts when multiple tabs are open on the same site, and some duplicate scripts even with single tabs on complex sites like techcrunch (e.g. from facebook/twitter iframes).

To test the effectiveness of this patch I opened 5 tabs on the nytimes.com homepage, minimized memory usage then ran about:memory.  With this patch the script-sources measurement went from 5.7 MB down to 1.9 MB, with the compressed source table only using 16 KB.  The size of the uncompressed source cache and associated main thread decompression overhead should also go down in this sort of situation since ScriptSources which are grouped together only need to be decompressed once instead of individually.  This is hard to measure though since this cache is cleared on GC (as an aside the size of the cache spikes after the GC due to what looks like lazy parsing of timer scripts which were presumably relazified by the GC.  I wonder what effect relazification has on the size of the cache in practice.)

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8433724 [details] [diff] [review]
patch

Review of attachment 8433724 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks great; nice job!  r+ assuming the bug fixed as described below.

::: js/public/OldDebugAPI.h
@@ +71,5 @@
>  
>    private:
>      void operator=(const FrameDescription &) MOZ_DELETE;
>  
> +    JSRuntime *runtime;

Can you add a _ suffix to match all the other members?

::: js/src/jit/AsmJSModule.h
@@ +462,5 @@
>      // with caching.
>      uint32_t                              funcStart_;
>      uint32_t                              offsetToEndOfUseAsm_;
>  
> +    JSRuntime *                           runtime_;

IIUC, there is a bug here: we'll create the AsmJSModule on the parse thread, when cx->maybeRuntime() == null.  Then the parse task will get finished on the main thread, the ScriptSource will get inserted into the compressed source set, and then ~AsmJSModule() will get called when associated object is finalized and will crash in scriptSource_->decref(runtime_).

I think the fix is pretty simple:
 - remove runtime_ from AsmJSModule
 - turn ~AsmJSModule into AsmJSModule::destroy(JSRuntime *rt), ending with js_delete(this)
 - change AsmJSModuleObject_finalize to call destroy, passing rt

::: js/src/jscntxt.h
@@ +214,5 @@
>  
>      bool isForkJoinContext() const;
>      ForkJoinContext *asForkJoinContext();
>  
> +    JSRuntime *maybeRuntime() const {

'maybe' usually means "I may or may not have this" and all cx's have a runtime, so it'd be nice to indicate the criteria used to return null.  How about 'runtimeIfOnMainThread'?

::: js/src/jsscript.cpp
@@ +1586,5 @@
> +
> +    CompressedSourceSet::AddPtr p = rt->compressedSourceSet.lookupForAdd(this);
> +    if (p) {
> +        // There is another ScriptSource with the same compressed data.
> +        // Mark this ScriptSource as the parent and use it for all attempts to

'this' is ambiguous in the comment and originally I read it the wrong way.

@@ +1733,2 @@
>  {
> +    JS_ASSERT_IF(inCompressedSourceSet, dataType == DataCompressed);

Perhaps also add
  JS_ASSERT_IF(inCompressedSourceSet, maybert);
I know it'll trivially crash below, but it seems nice to see it stated as a precondition at the top.

::: js/src/jsscript.h
@@ +605,5 @@
> +    static HashNumber hash(const ScriptSource *ss) {
> +        // Only hash based on the length of the compressed data and the last
> +        // N bytes, to avoid expensive lookups. The compressed data will be
> +        // influenced by earlier source in the uncompressed string so this
> +        // should still give a reasonable hash of that string's contents.

IIUC, most compression algos, incl zlib, use a sliding window that only refers back a fixed distance in the byte stream.  I think zlib's is 32K, but I could be wrong.  Anyhow, that means that the last 200 bytes are really only representative of the last 32K.  Having the length be part of the hash is a good mitigating factor.  Also, a collision isn't the end of the world unless we get a really long collision chain, which also seems unlikely.  So do we think this could actually happen in practice?  A 100 tab users might be able to load enough scripts to create a bad chain; the big question is if sites do things like version-stamp scripts (which would produce same-length scripts with only a few chars different).  Have you measured this hashing time as being significant?

::: js/src/vm/HelperThreads.cpp
@@ +701,5 @@
>  
>          // The NewScript hook needs to be called for all compiled scripts.
>          CallNewScriptHookForAllScripts(cx, script);
> +
> +        // Update the compressed source table with the result.

Could you add:
 "This is normally called by setCompressedSource when compilation occurs on the main thread."

Comment 2

[Nicholas Nethercote [inactive]]

> Have you measured this hashing time as being significant?

A SplayTree might be a reasonable alternative for this data structure.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: updated

I didn't measure the hashing overhead when writing the patch, but just did on the nytimes.com x 5 workload.  If we hash the entire compressed source then the total hashing time is a little over 5 ms for me, which isn't huge but is I think still enough to pay some attention to.  The attached patch changes the hashing strategy so that the entire compressed source is hashed, but also stores that hash on the ScriptSource so that the computation of the hash value is done immediately after the data is compressed, while we are still on the helper thread doing the compression.

This patch also fixes the AsmJSModule destruction issue.  Unfortunately AsmJSModules are also deleted via ScopedJSDeletePtrs, which need to be changed to use destroy(), and even more unfortunately I had to write a custom class for this destruction since we need to store and pass in the runtime pointer to destroy(), which mozilla::Scoped doesn't seem to be able to handle.

So, submitting this patch for rereview for the above two changes.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8439501 [details] [diff] [review]
updated

Review of attachment 8439501 [details] [diff] [review]:
-----------------------------------------------------------------

Nice solution with the hashing-off-main-thread.  Fortunately, I think we can use Scoped.h after all:

::: js/src/jit/AsmJSModule.h
@@ +555,5 @@
> +        {}
> +
> +        ~ScopedDestroyPtr() {
> +            if (module_)
> +                module_->destroy(nullptr);

On first glance, always passing null here looks like a bug.  However, thinking about it more, (1) even runtime_ can be null in the async parsing case, (2) passing null unconditionally here is fine since ScopedJSDeletePtrs are never held long enough that the owned AsmJSModule can possibly hold the last reference; the only possible time the AsmJSModule could hold the last ref is the module-object finalizer, which passes fop->runtime().  That means we don't need JSRuntime* here and you should be able to reuse mfbt/Scoped.h after all.  Assuming that works, could you switch back the .get()/.set() to operator*/operator=?

Comment 5

[Brian Hackett [Laid off!]]

Yeah, passing null there is a bug.  Earlier I was using mozilla::Scoped and always passing null for the runtime with the rationale you describe, but wasn't so happy with that because it's a difficult argument to precisely state and is really only needed to avoid 20 lines of boilerplate code.  But, sure, I'll go back to that approach.

Comment 6

[Luke Wagner [:luke]]

(In reply to Brian Hackett (:bhackett) from comment #5)
> but wasn't so happy with that because it's a difficult argument to precisely
> state

You're right, and I felt a little bad making it.  However, the argument for why it is sometimes ok to pass a null JSRuntime* to the ScopedDestroyPtr constructor is similar: one has to reason about the lifetime of the ScopedDestroyPtr wrt outstanding references on the ScriptSource.  An alternative is to revert all this and just use TLS inside ~AsmJSModule() to find the runtime when we're on the main thread (passing null when we're off).  I don't have any strong preferences.

Comment 7

[Brian Hackett [Laid off!]]

Ooh, yeah, using TlsPerThreadData in the ScriptSource destructor will be much cleaner, this passing-the-runtime-around stuff is going too far down the rabbit hole.

Comment 8

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28357f609779

Comment 9

[Luke Wagner [:luke]]

Mmm, much nicer.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28357f609779