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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1290108-refcount-ssd

Comment 2

[Jon Coppeard (:jonco)]

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 3

[Terrence Cole [:terrence]]

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/

Comment 4

[Jon Coppeard (:jonco)]

(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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92a9c724b77f
Make SharedScriptData refcounted so we can free them without doing a full GC r=terrence

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92a9c724b77f