661903 - Move script filename table to the compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This is the last thing to move to the compartment. Once it's done, everything swept by the GC will be compartment-local. This will make it easier for all GCs to be compartment GCs.

Igor, there's one thing I don't understand about this code. Why do we mark all the filenames if gcKeepAtoms is true? This behavior seems to have been introduced in bug 291312. I'm guessing this code is needed if we create a script and GC before the script is rooted. It would be nice to take it out if it's not needed, or maybe fix it in a more elegant way (by rooting the script). There are a lot of uses of gcKeepAtoms, and most of them don't seem to be related to script filenames, as far as I can tell.

Comment 1

[Igor Bukanov]

Comment on attachment 537198 [details] [diff] [review]
patch

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

r+ with the comment below addressed.

As regarding removal of comp->rt->gcKeepAtoms checks lets file a new bug about it. I think you are right and this is not necessary, but lets keep the issues separated.

::: js/src/jsscript.cpp
@@ +793,5 @@
>  
> +    ScriptFilenameTable::AddPtr p = comp->scriptFilenameTable.lookupForAdd(filename);
> +    if (!p) {
> +        size_t size = offsetof(ScriptFilenameEntry, filename) + strlen(filename) + 1;
> +        ScriptFilenameEntry *entry = (ScriptFilenameEntry *) OffTheBooks::malloc_(size);

Use cx->malloc_ here as we allocate memory that can only be released during the GC. This also removes the call to JS_ReportOutOfMemory(cx);

@@ +838,4 @@
>  }
>  
>  void
> +js_MarkScriptFilenames(JSCompartment *comp)

Remove this function. Instead in comp->rt->gcKeepAtoms free the entry only when comp->rt->gcKeepAtoms is false.

Comment 3

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9a325ccad497

Comment 4

[(no longer active)]

Oh, also: http://hg.mozilla.org/mozilla-central/rev/c5bddfd1bbe1