The default bug view has changed. See this FAQ.

Move script filename table to the compartment

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
Attachment #537198 - Flags: review?(igor)

Comment 1

6 years ago
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.
Attachment #537198 - Flags: review?(igor) → review+
Duplicate of this bug: 567202
Blocks: 647367
(Assignee)

Updated

6 years ago
Assignee: general → wmccloskey
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/9a325ccad497
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Oh, also: http://hg.mozilla.org/mozilla-central/rev/c5bddfd1bbe1
Blocks: 718053

Updated

5 years ago
Depends on: 643967
You need to log in before you can comment on or make changes to this bug.