Closed
Bug 802318
Opened 12 years ago
Closed 11 years ago
GC: remove skip-roots from jsatom.cpp
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: terrence, Assigned: terrence)
References
Details
(Whiteboard: [js:t])
Attachments
(1 file)
11.82 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
At the moment, AtomizeInline accepts a jschar* which may point into an inline string. Forcing this to take a StableCharPtr would, quite unacceptably, force all atoms to be stable strings. Instead, we should reorganize this as following: (1) AtomizeChars should take a StableCharPtr or callers should get rewritten to use AtomizeString; (2) AtomizeString should be rewritten such that it only needs to extract characters after allocating the new Atom, allowing even short/inline strings to work here.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [js:t]
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
This is largely code motion to allow the Copy and TakeOwnership to specialize on the stableness and inlinedness of the string. A nice knock-on effect from this is that AtomizeString no longer needs to stabilize inline strings.
Comment on attachment 698829 [details] [diff] [review] v0 Review of attachment 698829 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsatom.cpp @@ +242,4 @@ > */ > JS_ALWAYS_INLINE > +static UnrootedAtom > +AtomizeInlineTakeOwnership(JSContext *cx, StableCharPtr tbchars, size_t length, How about AtomizeAndTakeOwnership? @@ +249,3 @@ > > + if (UnrootedAtom s = cx->runtime->staticStrings.lookup(tbchars.get(), length)) { > + JS_free(cx, (void*)tbchars.get()); Please just use js_free here and for the other JS_free calls below. @@ +255,5 @@ > + /* > + * If a GC occurs at js_NewStringCopy then |p| will still have the correct > + * hash, allowing us to avoid rehashing it. Even though the hash is > + * unchanged, we need to re-lookup the table position because a last-ditch > + * GC will unlock the atomsCompartment and potentially free some table Please remove the phrase "unlock the atoms compartment and" since I don't even know what it means. @@ +288,5 @@ > +} > + > +JS_ALWAYS_INLINE > +static UnrootedAtom > +AtomizeInlineCopy(JSContext *cx, const jschar *tbchars, size_t length, InternBehavior ib) How about AtomAndCopyStableChars? Also, please put a comment above this function saying that tbchars should not point into an inline JSString. @@ +297,5 @@ > + /* > + * If a GC occurs at js_NewStringCopy then |p| will still have the correct > + * hash, allowing us to avoid rehashing it. Even though the hash is > + * unchanged, we need to re-lookup the table position because a last-ditch > + * GC will unlock the atomsCompartment and potentially free some table Same issue with the comment. @@ +319,4 @@ > > /* > + * We re-generate the Lookup from |flat| here because |tbchars| may be > + * inline in a moved JSString and is thus not safe to use after Please revise this comment. As we discussed, this function won't work if tbchars points into an inline JSString. @@ +350,5 @@ > p->setTagged(bool(ib)); > return &atom; > } > > + JSLinearString *linear = str->ensureLinear(cx); As we discussed, this needs to remain ensureStable.
Attachment #698829 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 3•11 years ago
|
||
Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=bc8efe416481 Pushed at: https://hg.mozilla.org/integration/mozilla-inbound/rev/c420b26e7e84
Comment 4•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c420b26e7e84
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•