GC: specialize low-level character access for JSStableString

RESOLVED FIXED in mozilla19

Status

()

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 773686
(Assignee)

Comment 1

6 years ago
Created attachment 668658 [details] [diff] [review]
v0

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 2

6 years ago
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+
(Assignee)

Comment 3

6 years ago
(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.

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 669278 [details] [diff] [review]
v0: Fix for a busted assertion in RangedPtr.h.

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+
(Assignee)

Comment 6

6 years ago
Created attachment 669300 [details] [diff] [review]
nits

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+
(Assignee)

Comment 8

6 years ago
And backed out for CTypes build failure in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14684be81166
https://hg.mozilla.org/mozilla-central/rev/2c08d52e521d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

6 years ago
Depends on: 837845

Comment 11

6 years ago
Should this bug be reopened because of the backout landed in bug 837845?
(Assignee)

Comment 12

6 years ago
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.
Blocks: 845713

Updated

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