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

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1288715
(Assignee)

Comment 1

2 years ago
Created attachment 8775611 [details] [diff] [review]
bug1290108-refcount-ssd
Attachment #8775611 - Flags: review?(terrence)
(Assignee)

Comment 2

2 years ago
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+
(Assignee)

Comment 4

2 years ago
(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

2 years ago
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

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92a9c724b77f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.