Closed Bug 1418844 Opened 3 years ago Closed 3 years ago

Cleanup SharedScriptData matching

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
This patch makes the SharedScriptData hold the srcnotes accessor instead of cleverness on JSScript.
Assignee: nobody → tcampbell
Status: NEW → ASSIGNED
Attachment #8929897 - Flags: review?(jdemooij)
This rewrites the match logic for deduplicating SharedScriptData to check lengths of individual regions instead of simply the total length. Avoids theoretical collisions.
Attachment #8929898 - Flags: review?(jdemooij)
Summary: Cleanup ShareScriptData matching → Cleanup SharedScriptData matching
Priority: -- → P2
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.
Attachment #8929897 - Flags: review?(jdemooij) → review+
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.
Attachment #8929898 - Flags: review?(jdemooij) → review+
Nits addressed. Forwarding r+ from Comment 4.
Attachment #8929897 - Attachment is obsolete: true
Attachment #8930151 - Flags: review+
Keywords: checkin-needed
Pushed by ncsoregi@mozilla.com:
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
Keywords: checkin-needed
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad92db41819
followup - Add message to static_assert. r=red
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7954209d276d
followup - Use pre-decrement instead of post-decrement to fix leaks. r=tcampbell
You need to log in before you can comment on or make changes to this bug.