Closed Bug 798624 Opened 8 years ago Closed 8 years ago
GC: specialize low-level character access for JSStable
If we specialize JSStableString::chars() and push StableCharPtr into the interface at several key choke-points we can use the type system to enforce the requirement that we have uninlined jschar*s in places that we may GC with a jschar* on the stack.
This handles the simple cases. There is still RegExpObject, but it requires an atom, so is going to need a more involved change. My plan is to implement JSStableAtom and refactor AtomizeInline [/me shudders], but that can come later.
Attachment #668658 - Flags: review?(luke)
Comment on attachment 668658 [details] [diff] [review] v0 Review of attachment 668658 [details] [diff] [review]: ----------------------------------------------------------------- This is great: typier and assertier! My nits are basically just to push StableCharPtr into a few more interfaces that currently take 'const jschar *': (I assume UnstableCharPtr use is coming later?) ::: js/src/frontend/BytecodeCompiler.cpp @@ +94,5 @@ > case CompileOptions::NO_SOURCE: > break; > } > > + Parser parser(cx, options, chars.get(), length, /* foldConstants = */ true); Parser may want to store a const jschar * internally, but it makes sense for it to take a StableCharPtr in the constructor interface. ::: js/src/jsclone.cpp @@ +1004,5 @@ > return false; > > size_t length = stable->length(); > + const StableCharPtr chars = stable->chars(); > + RegExpObject *reobj = RegExpObject::createNoStatics(context(), chars.get(), length, flags, NULL); For the same reason about, it seems like RegExpObject::createNoStatics would take a stable pointer (as a semantic interface requirement) and use .get() internally. ::: js/src/jscompartment.cpp @@ +279,5 @@ > RootedValue orig(cx, *vp); > JSStableString *str = vp->toString()->ensureStable(cx); > if (!str) > return false; > + JSString *wrapped = js_NewStringCopyN(cx, str->chars().get(), str->length()); ditto
Attachment #668658 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #2) > (I assume UnstableCharPtr use is coming later?) That is the hope, but I'm not sure how likely it is anymore. My previous effort to convert all the jschar* interfaces at once was starting to get seriously overweight. When I realized we could get most of the benefits using, well, this patch, I decided to split this off and get it in first. Converting all of our internal jschar* use to RangedPtr is a noble goal, but it isn't on the shortest path to exact rooting anymore. I could include it to make things easier to future or remove it since it's unused in the interim - which would be better? > ::: js/src/jscompartment.cpp > @@ +279,5 @@ > > RootedValue orig(cx, *vp); > > JSStableString *str = vp->toString()->ensureStable(cx); > > if (!str) > > return false; > > + JSString *wrapped = js_NewStringCopyN(cx, str->chars().get(), str->length()); > > ditto Ah, good catch! This one hadn't occurred to me because most of the users are for buffers.
(In reply to Terrence Cole [:terrence] from comment #3) > That is the hope, but I'm not sure how likely it is anymore. My previous > effort to convert all the jschar* interfaces at once was starting to get > seriously overweight. I bet. I wasn't asking for internal use; more for JSAPI public use. If we don't do the latter, then perhaps we could leave UnstableCharPtr out.
In RegExpExecute we have: size_t displacement = 0; .... stuff that may or may not modify displacement .... chars += displacement; RangedPtr.h is asserting on this code in operator+/- that the offset is strictly > 0, which is just wrong. Setting rs=Waldo from IRC.
Attachment #669278 - Flags: superreview+
The nit corrections turned into a 40k patch. There is nothing controversial in it, I'm just putting it here for completeness' sake. I also opened new bug 799248. Making NewString GC safe is going to need another review pass and I don't want to make you have to wade through all of these mechanical changes a second time.
Attachment #669300 - Flags: review+
And backed out for CTypes build failure in: https://hg.mozilla.org/integration/mozilla-inbound/rev/14684be81166
Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=68b7150624f1 Pushed at: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c08d52e521d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Should this bug be reopened because of the backout landed in bug 837845?
Don't bother. The real solution to this problem is considerably more labor intensive, which is why we took this path in the first place. Until then we will simply not store strings in the nursery. I'll open a new blocking bug later when we have the time to work on that.
You need to log in before you can comment on or make changes to this bug.