Last Comment Bug 661903 - Move script filename table to the compartment
: Move script filename table to the compartment
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Bill McCloskey (:billm)
:
Mentors:
: 567202 (view as bug list)
Depends on: 643967
Blocks: 647367 718053
  Show dependency treegraph
 
Reported: 2011-06-03 11:52 PDT by Bill McCloskey (:billm)
Modified: 2012-02-21 12:44 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (16.34 KB, patch)
2011-06-03 11:52 PDT, Bill McCloskey (:billm)
igor: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2011-06-03 11:52:22 PDT
Created attachment 537198 [details] [diff] [review]
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 2011-06-03 13:43:17 PDT
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 2 Chris Leary [:cdleary] (not checking bugmail) 2011-06-28 16:16:08 PDT
*** Bug 567202 has been marked as a duplicate of this bug. ***
Comment 4 :Ehsan Akhgari 2011-07-15 07:07:57 PDT
Oh, also: http://hg.mozilla.org/mozilla-central/rev/c5bddfd1bbe1

Note You need to log in before you can comment on or make changes to this bug.