Closed Bug 1290108 Opened 4 years ago Closed 4 years ago

Refcount shared script data so we can free it outside a full GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

SharedScriptData objects can be referenced by JSScripts in any zone and are marked and swept in full GCs.  If we reference count them instead then we can free them without doing a full GC.
Depends on: 1288715
Attachment #8775611 - Flags: review?(terrence)
Comment on attachment 8775611 [details] [diff] [review]
bug1290108-refcount-ssd

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

::: js/src/jsgc.cpp
@@ +5428,5 @@
>           * Sweep script filenames after sweeping functions in the generic loop
>           * above. In this way when a scripted function's finalizer destroys the
>           * script and calls rt->destroyScriptHook, the hook can still access the
>           * script's filename. See bug 323267.
>           */

BTW I have no idea what this comment is doing here.  I think it's talking about finalization phases and why strings are always (mostly) finalized after scripts.  It should probably go somewhere else.
Comment on attachment 8775611 [details] [diff] [review]
bug1290108-refcount-ssd

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

This is *so* much nicer!

::: js/src/jsscript.cpp
@@ +2425,3 @@
>      entry->dataLength_ = dataLength;
>      entry->natoms_ = natoms;
>      entry->codeLength_ = codeLength;

Would be nice to move all of this to a constructor. Not for this patch though.

@@ +2530,5 @@
>  
> +    for (ScriptDataTable::Enum e(table); !e.empty(); e.popFront()) {
> +#ifdef DEBUG
> +        SharedScriptData* scriptData = e.front();
> +        fprintf(stderr, "ERROR: GC found live SharedScriptData %p with ref count %d at shutdown\n",

It would be nice to just assert this, otherwise it will probably pass undetected on try. I guess the worry is that crashing the process will make it harder to debug?

@@ -3914,5 @@
> -     * a GC. Since SweepScriptBytecodes is only called during a full gc,
> -     * to preserve this invariant, only mark during a full gc.
> -     */
> -    if (trc->isMarkingTracer() && trc->runtime()->gc.isFullGc())
> -        setMarked(true);

\o/ \o/ \o/
Attachment #8775611 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #3)

> @@ +2530,5 @@
> >  
> > +    for (ScriptDataTable::Enum e(table); !e.empty(); e.popFront()) {
> > +#ifdef DEBUG
> > +        SharedScriptData* scriptData = e.front();
> > +        fprintf(stderr, "ERROR: GC found live SharedScriptData %p with ref count %d at shutdown\n",
> 
> It would be nice to just assert this, otherwise it will probably pass
> undetected on try. I guess the worry is that crashing the process will make
> it harder to debug?

This would add another place where we crash in shutdown if the embedding leaks JS things.  I think we need a strategy to deal with asserting our invariants hold where we can without being tripped up by external factors like this.  I'll file another bug for this.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92a9c724b77f
Make SharedScriptData refcounted so we can free them without doing a full GC r=terrence
https://hg.mozilla.org/mozilla-central/rev/92a9c724b77f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.