799248 - GC: remove skip-roots from NewString function family

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

The NewString functions need to allocate (and therefore GC) before using their jschar* argument.  If we make the NewString functions take StableCharPtr, we will basically kill off our ability to ever use inline strings.  Since these functions just need to be able to find the base of the character buffer after a GC, the right solution is some form of RelocatableCharPtr.

Comment 1

[Terrence Cole [:terrence]]

Waldo points out that NewString should just take a JSString/offset/length in any of the places where we do not already have a StableCharPtr/StableString.  I concur that this is a better solution.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v0

This patch contains a couple of simple, but very helpful cleanups.

1) Create a new StableCharPtr for each of the jschar* JS-API inputs, similar to how we root everything internally.  We were already doing this in an ad-hoc manner for the interfaces that require StableCharPtr, this just makes everything consistent.

2) Convert all of the optionally NULL |cx| parameters in the Inflate/Deflate function family to be named |maybecx|.  This also moves the local variable declarations down to where they should be in C++.

3) PodCopy/pod_malloc in js_NewStringCopyZ.

Sorry to hand you such a boring, mechanical review, but I wanted to keep these changes from cluttering up the real patch, which I will attach shortly.

Comment 3

[Terrence Cole [:terrence]]

Attachment: v0: The fixes.

This is the first half of making our JSString creation functions take StableCharPtrs.  The second half is going to require significant and orthogonal changes to Atomize, so I've split it off into bug 802318.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 672042 [details] [diff] [review]
v0: The fixes.

Hurm, try run is busted, I'd better investigate first.

Comment 5

[Terrence Cole [:terrence]]

Attachment: v1

Looks green now: https://tbpl.mozilla.org/?tree=Try&rev=dc3604cbc8fe

The failure was that I was not updating the start offset when climbing to the most basal dependent string in js_NewDependentString.  This version also adds StableCharPtr for JSExtendedString, which I missed before, and pushes StableCharPtr into the various init methods.

The last step now is the make pod_malloc<T>(size_t) return a RangedPtr<T>.  I still need to figure out how to make RangedPtr handle the |const jschar| vs |jschar| distinction elegantly.  Other than that, it looks fairly straightforward to do this now, given that most of the users flow directly into a StableCharPtr.

Comment 6

[Luke Wagner [:luke]]

This has been bugging me for a while and Waldo was thinking the same thing:

When an API logically wants to take (pointer, length), and then we go off and make it safe, so we have to write (RangedPtr(pointer, length), length).  The second patch here does this all over the place.

So, my suggesting: wouldn't it be nice if we could have a new Range<T> type that represented a (pointer, length).  Then, it could have a .ptr() method that returned a RangedPtr<T> and a .length().  Since the above patches really wants Range<T> to not look dorky, could we do that as a prereq for this bug?

Comment 7

[Terrence Cole [:terrence]]

I've been thinking about doing this myself.  I held off because I was going to make pod_malloc(size_t) return RangedPtr next.  This would kill off almost all of these places, leaving us with a rather minimal interface diff from |const jschar *| to |StableCharPtr|.  

If you're asking for this change now, however....  I'll just go polish my code-carving tools for a bit.

One question I have is: Is Range<T> the best name for this tuple?  IMHO, I'd rather put the length directly into the CharPtr heirarchy.  It's less general, but I think CharPtr is a broadly applicable enough class that it warrants this treatment.

Comment 8

[Luke Wagner [:luke]]

I'd expect:

  RangedPtr <-- CharPtr <-- StableCharPtr
  Range <-- CharRange <-- StableCharRange

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 672040 [details] [diff] [review]
v0

Whoops, sorry I left this flagged so long, Luke.

Comment 10

[Terrence Cole [:terrence]]

Attachment: v2

Now that TwoByteChars has landed, we can do this much more simply. This extends the existing infrastructure just enough to allow us to safely create short strings from Unstable chars.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 702587 [details] [diff] [review]
v2

Review of attachment 702587 [details] [diff] [review]:
-----------------------------------------------------------------

Looks nice, thanks.

::: js/public/CharacterEncoding.h
@@ +94,5 @@
>      TwoByteChars(const jschar *chars, size_t length) : Base(const_cast<jschar *>(chars), length) {}
>  };
>  
>  /*
> + * A non-convertible varient of TwoByteChars that does not refer to characters

variant

::: js/src/vm/String-inl.h
@@ +21,5 @@
>  
>  namespace js {
>  
>  static JS_ALWAYS_INLINE JSInlineString *
> +NewShortString(JSContext *cx, Latin1Chars chars)

Why does this need to be inline when it wasn't before?

@@ +76,5 @@
> +    JSInlineString *str = JSInlineString::lengthFits(len)
> +                          ? JSInlineString::tryNew_(cx)
> +                          : JSShortString::tryNew_(cx);
> +    if (!str) {
> +        ScopedFreePtr<jschar> tmp(cx->pod_malloc<jschar>(len));

Why malloc? Can't you stack allocate it?

Comment 12

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #11)
> Comment on attachment 702587 [details] [diff] [review]
> ::: js/src/vm/String-inl.h
> @@ +21,5 @@
> >  
> >  namespace js {
> >  
> >  static JS_ALWAYS_INLINE JSInlineString *
> > +NewShortString(JSContext *cx, Latin1Chars chars)
> 
> Why does this need to be inline when it wasn't before?

Yeah, this is pretty annoying. For reasons I don't entirely understand, without the inline clang gives an unused function warning on every translation unit vm/String-inl.h gets included into except for jsstr.cpp, where it is used.

Our options are:
 - Move JSDependentString::new_ out of the header so that we can move NewShortString out of headers as well.
 - Move NewShortString(Latin1) back to where it was, not alongside its namesake.
 - Make it inline. This doesn't hurt us since it only has one caller anyway.

I think that inlining is probably the best tradeoff here.

Comment 13

[Terrence Cole [:terrence]]

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=0567c8145d5b

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27f62505f15c

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/27f62505f15c