1418844 - Cleanup SharedScriptData matching

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1418844-1-Make-srcnote-data-explicit-in-SharedSc.patch

This patch makes the SharedScriptData hold the srcnotes accessor instead of cleverness on JSScript.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: 0002-Bug-1418844-2-Check-all-lengths-in-ScriptBytecodeHas.patch

This rewrites the match logic for deduplicating SharedScriptData to check lengths of individual regions instead of simply the total length. Avoids theoretical collisions.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8929897 [details] [diff] [review]
0001-Bug-1418844-1-Make-srcnote-data-explicit-in-SharedSc.patch

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

Oh, wow. This is much nicer and simpler.

::: js/src/jsscript.cpp
@@ +2435,5 @@
>      /*
>       * Call constructors to initialize the storage that will be accessed as a
>       * GCPtrAtom array via atoms().
>       */
> +    JS_STATIC_ASSERT(offsetof(SharedScriptData, data_) % sizeof(GCPtrAtom) == 0);

Nit: use static_assert directly (JS_STATIC_ASSERT is deprecated)

@@ +2441,4 @@
>      for (unsigned i = 0; i < natoms; ++i)
>          new (&atoms[i]) GCPtrAtom();
>  
>      return entry;

Would be nice to add this assert somewhere in this function:

  MOZ_ASSERT(entry->dataLength() == dataLength);

To make sure the dataLength calculations here and in dataLength() stay consistent.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8929898 [details] [diff] [review]
0002-Bug-1418844-2-Check-all-lengths-in-ScriptBytecodeHas.patch

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

::: js/src/jsscript.h
@@ +851,5 @@
>  };
>  
>  struct ScriptBytecodeHasher
>  {
> +    typedef SharedScriptData Lookup;

I was wondering if we could somehow end up copying SharedScriptData, but we delete its copy constructor so this should be fine.

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1418844-1-Make-srcnote-data-explicit-in-SharedSc.patch

Nits addressed. Forwarding r+ from Comment 4.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13cf30f1a551
(1) Make srcnote data explicit in SharedScriptData. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb626ee6ff4
(2) Check all lengths in ScriptBytecodeHasher. r=jandem

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad92db41819
followup - Add message to static_assert. r=red

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7954209d276d
followup - Use pre-decrement instead of post-decrement to fix leaks. r=tcampbell

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/13cf30f1a551
https://hg.mozilla.org/mozilla-central/rev/5fb626ee6ff4
https://hg.mozilla.org/mozilla-central/rev/5ad92db41819
https://hg.mozilla.org/mozilla-central/rev/7954209d276d