Created attachment 267531 [details]
Patch

Comment on attachment 267531 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267531&action=review

OK to land as is. I have some suggestions that I think might make the code a little nicer.

> Source/JavaScriptCore/ChangeLog:19
> +        Remove the "operator StringView()" convenience helper on JSString::SafeString since that
> +        made it possible to casually turn the return value from JSString::view() into an unsafe
> +        StringView local on the stack with this pattern:
> +
> +            StringView view = someJSValue.toString(exec)->view(exec);
> +
> +        The JSString* returned by toString() above will go out of scope by the end of the statement
> +        and does not stick around to protect itself from garbage collection.
> +
> +        It will now look like this instead:
> +
> +            JSString::SafeView view = someJSValue.toString(exec)->view(exec);

Shouldn’t we normally just have the local variable be the JSString*? How often do we really want the StringView as a local instead of a function argument?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:190
> +    JSString* volatile separatorString;

Is this really needed/helpful?

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:153
> -    StringView str = exec->argument(0).toString(exec)->view(exec);
> +    JSString::SafeView str = exec->argument(0).toString(exec)->view(exec);

I suggest refactoring this (adding another level of function call, inline if you like) so the StringView can be an argument instead of a local variable. I hate actually having to utter the type name JSString::SafeView.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:620
> +    JSString::SafeView s = value.toString(exec)->view(exec);

I suggest having the local variable be a JSString*, not a JSString::SafeView.

    JSString* string = value.toString(exec);

Then below, string->view(exec).get() instead of s.get().

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:692
> +    JSString::SafeView str = exec->argument(0).toString(exec)->view(exec);

Can we refactor this so the StringView is an argument to a separate function?

Also seems strange that doNotEscape is not marked const!

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:730
> +    JSString::SafeView str = exec->argument(0).toString(exec)->view(exec);

Can we refactor this so the StringView is an argument to a separate function?

> Source/JavaScriptCore/runtime/JSONObject.cpp:732
> +    JSString::SafeView source = exec->uncheckedArgument(0).toString(exec)->view(exec);

Can we refactor this so the StringView is an argument to a separate function?

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1933
> +    JSString::SafeView source = thisValue.toString(exec)->view(exec);

Seems like we should refactor so we have a second function that just takes two StringView objects.

(In reply to comment #2)
> Comment on attachment 267531 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267531&action=review
> 
> OK to land as is. I have some suggestions that I think might make the code a
> little nicer.
> 
> > Source/JavaScriptCore/ChangeLog:19
> > +        Remove the "operator StringView()" convenience helper on JSString::SafeString since that
> > +        made it possible to casually turn the return value from JSString::view() into an unsafe
> > +        StringView local on the stack with this pattern:
> > +
> > +            StringView view = someJSValue.toString(exec)->view(exec);
> > +
> > +        The JSString* returned by toString() above will go out of scope by the end of the statement
> > +        and does not stick around to protect itself from garbage collection.
> > +
> > +        It will now look like this instead:
> > +
> > +            JSString::SafeView view = someJSValue.toString(exec)->view(exec);
> 
> Shouldn’t we normally just have the local variable be the JSString*? How
> often do we really want the StringView as a local instead of a function
> argument?

What guarantees that a local JSString* will stick around on the stack or in a register? Isn't the compiler free to re-use that slot for optimization purposes after it's no longer needed?

(In reply to comment #3)
> What guarantees that a local JSString* will stick around on the stack or in
> a register? Isn't the compiler free to re-use that slot for optimization
> purposes after it's no longer needed?

Gotta admit you are right. You just blew my mind. How could I have invented SafeView in the first place and not realized how seriously important it is. I’m not sure that volatile will even make us safe.

(In reply to comment #3)
> What guarantees that a local JSString* will stick around on the stack or in
> a register? Isn't the compiler free to re-use that slot for optimization
> purposes after it's no longer needed?

OK, so on thinking about it more, this means that we can’t pass a StringView to a function that does any GC heap memory allocation. So it's not safe to call view(exec).get() when calling a function that takes a StringView that can do any GC heap memory allocation. I guess those arguments need to be JSString::SafeView instead.

Comment on attachment 267531 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267531&action=review

>>> Source/JavaScriptCore/ChangeLog:19
>>> +            JSString::SafeView view = someJSValue.toString(exec)->view(exec);
>> 
>> Shouldn’t we normally just have the local variable be the JSString*? How often do we really want the StringView as a local instead of a function argument?
> 
> What guarantees that a local JSString* will stick around on the stack or in a register? Isn't the compiler free to re-use that slot for optimization purposes after it's no longer needed?

OK, here’s the thing: If the local variable is a JSString*, then it is going to be used for the call to view(exec), and the SafeView will stay around long enough. So my suggestion is fine. It’s not like we will have a JSString* that isn’t used any more. This only applies if we call view(exec) on the JSString*, not trying to use the JSString* as some kind of “protector”.

The rule of thumb is that view(exec).get() should always be called in a function call, to initialize a local variable.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:190
>> +    JSString* volatile separatorString;
> 
> Is this really needed/helpful?

Another way to do this is to refactor so we are calling a helper function. Then we don’t need a local variable for separator; it can just be an argument to the function that does the rest of the joining work. It would be nice to avoid the use of volatile. Way too subtle, I think.

(In reply to comment #5)
> (In reply to comment #3)
> > What guarantees that a local JSString* will stick around on the stack or in
> > a register? Isn't the compiler free to re-use that slot for optimization
> > purposes after it's no longer needed?
> 
> OK, so on thinking about it more, this means that we can’t pass a StringView
> to a function that does any GC heap memory allocation. So it's not safe to
> call view(exec).get() when calling a function that takes a StringView that
> can do any GC heap memory allocation. I guess those arguments need to be
> JSString::SafeView instead.

I wonder if we could bake this into StringView itself somehow, e.g by adding a volatile void* member that can store the origin JSString in these cases?

(In reply to comment #6)
> Comment on attachment 267531 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267531&action=review
> 
> >>> Source/JavaScriptCore/ChangeLog:19
> >>> +            JSString::SafeView view = someJSValue.toString(exec)->view(exec);
> >> 
> >> Shouldn’t we normally just have the local variable be the JSString*? How often do we really want the StringView as a local instead of a function argument?
> > 
> > What guarantees that a local JSString* will stick around on the stack or in a register? Isn't the compiler free to re-use that slot for optimization purposes after it's no longer needed?
> 
> OK, here’s the thing: If the local variable is a JSString*, then it is going
> to be used for the call to view(exec), and the SafeView will stay around
> long enough. So my suggestion is fine. It’s not like we will have a
> JSString* that isn’t used any more. This only applies if we call view(exec)
> on the JSString*, not trying to use the JSString* as some kind of
> “protector”.
> 
> The rule of thumb is that view(exec).get() should always be called in a
> function call, to initialize a local variable.

I don't understand what you mean here. Are you saying that view(exec).get() should only be used like so:

    foo(someJSString->view(exec).get())

The SafeString returned by view() here will always persist until foo() has returned, right?

(In reply to comment #7)
> (In reply to comment #5)
> > OK, so on thinking about it more, this means that we can’t pass a StringView
> > to a function that does any GC heap memory allocation. So it's not safe to
> > call view(exec).get() when calling a function that takes a StringView that
> > can do any GC heap memory allocation. I guess those arguments need to be
> > JSString::SafeView instead.
> 
> I wonder if we could bake this into StringView itself somehow, e.g by adding
> a volatile void* member that can store the origin JSString in these cases?

When I wrote this I was a little confused. I think my other comment is more to the point.

> (In reply to comment #6)
> > OK, here’s the thing: If the local variable is a JSString*, then it is going
> > to be used for the call to view(exec), and the SafeView will stay around
> > long enough. So my suggestion is fine. It’s not like we will have a
> > JSString* that isn’t used any more. This only applies if we call view(exec)
> > on the JSString*, not trying to use the JSString* as some kind of
> > “protector”.
> > 
> > The rule of thumb is that view(exec).get() should always be called in a
> > function call, to initialize a local variable.
> 
> I don't understand what you mean here. Are you saying that view(exec).get()
> should only be used like so:
> 
>     foo(someJSString->view(exec).get())
> 
> The SafeString returned by view() here will always persist until foo() has
> returned, right?

Exactly.

Because the lifetime of the JSString::SafeView is “until the end of the full expression”. Using the JSString::SafeView in an expression that is part of a function call effectively extends its lifetime until the function returns, which is exactly what we want. If the JSString::SafeView was in an expression on a line of code that initializes a local variable of some other type, then it only lives until the end of that line of code.

(In reply to comment #7)
> I wonder if we could bake this into StringView itself somehow, e.g by adding
> a volatile void* member that can store the origin JSString in these cases?

Sure, of course we would. But would it ruin StringView for other use cases?

(In reply to comment #6)
> Comment on attachment 267531 [details]
> Patch
> 
> >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:190
> >> +    JSString* volatile separatorString;
> > 
> > Is this really needed/helpful?
> 
> Another way to do this is to refactor so we are calling a helper function.
> Then we don’t need a local variable for separator; it can just be an
> argument to the function that does the rest of the joining work. It would be
> nice to avoid the use of volatile. Way too subtle, I think.

Agreed. This whole issue too subtle for comfort. Requiring volatile on a local for correctness clearly crosses a line, so let's do better.

There's a JSC::Local class that seems to solve this same problem using a Heap-allocated Handle to guard the pointee. I wonder if that is a more reliable solution than the marked-up raw pointer we're using in JSString::SafeView. I'd really love to know for sure what guarantees are made about 'volatile'.

(In reply to comment #10)
> There's a JSC::Local class that seems to solve this same problem using a
> Heap-allocated Handle to guard the point.

Too slow?

> I wonder if that is a more
> reliable solution than the marked-up raw pointer we're using in
> JSString::SafeView.

I don’t know of any actual practical reliability problems that would solve. But it would remove the dependency on volatile.

> I'd really love to know for sure what guarantees are
> made about 'volatile'.

I talked to Anders about it when I did this in the first place and he was pretty confident that the use of volatile in SafeView was OK. If you think about you’ll realize our garbage collector already relies on the compiler not doing anything crazy with pointers and optimization. I could imagine a compiler that did optimizations that meant that the pointer would not be there on the stack or in a register to scan when doing a GC.

But more importantly, I think if you reflect on it you’ll agree with me that our problems with this have not been with the use of volatile.

(In reply to comment #9)
> (In reply to comment #7)
> > I wonder if we could bake this into StringView itself somehow, e.g by adding
> > a volatile void* member that can store the origin JSString in these cases?
> 
> Sure, of course we would. But would it ruin StringView for other use cases?

Implementation wise it would only necessitate adding a member that gets assigned once and never checked for anything. There would be no ref/deref performance penalty.

It would grow StringView by one pointer though, which might make some call sites spill rather than passing in registers. (+CC andersca) 

This has a bitter taste of layering violation, which I don't like. Also we've already accepted the lifetime implications of StringViews pointing into C++ strings, so maybe we should do the same for GC strings. At the same time GC string lifetimes are far less obvious than C++ ones.

(In reply to comment #11)
> (In reply to comment #10)
> > There's a JSC::Local class that seems to solve this same problem using a
> > Heap-allocated Handle to guard the point.
> 
> Too slow?

It would be crazy for StringView construction to incur the cost of a Handle allocation. I was just idly thinking along the lines of "hey, maybe Local could be implemented using a volatile raw pointer instead of a Handle" :) Separate issue from all this.

> > I wonder if that is a more
> > reliable solution than the marked-up raw pointer we're using in
> > JSString::SafeView.
> 
> I don’t know of any actual practical reliability problems that would solve.
> But it would remove the dependency on volatile.
> 
> > I'd really love to know for sure what guarantees are
> > made about 'volatile'.
> 
> I talked to Anders about it when I did this in the first place and he was
> pretty confident that the use of volatile in SafeView was OK. If you think
> about you’ll realize our garbage collector already relies on the compiler
> not doing anything crazy with pointers and optimization. I could imagine a
> compiler that did optimizations that meant that the pointer would not be
> there on the stack or in a register to scan when doing a GC.
> 
> But more importantly, I think if you reflect on it you’ll agree with me that
> our problems with this have not been with the use of volatile.

Yep, totally agreed. I don't take particular issue with the use of volatile, it's just that I don't personally *know* why it works.

I'm gonna fix up the separator local thingy and land this patch. Then we can iterate on top of that if/when better ideas emerge.

> > There's a JSC::Local class that seems to solve this same problem using a
> > Heap-allocated Handle to guard the point.
> 
> Too slow?

Allocating a JSC::Local is designed to be reasonably fast -- but probably not much faster than allocating a WTF::String substring or incrementing a WTF::String refcount.

We haven't used JSC::Local much because we never switched to the kind of GC that requires an accurate stack. (And doing so is no longer in our long-term roadmap.) So, it's a bit untested.

If we want to keep an argument thing on the stack, perhaps we can do some lambda stuff:

JSString* string = ...;
string->view(exec, [](const StringView& view) {
    // Look ma, I can string!
});

But even here, the compiler is free to inline and undo everything we said to do.

Created attachment 267653 [details]
Patch for landing

Created attachment 267659 [details]
Patch for landing

Comment on attachment 267659 [details]
Patch for landing

Clearing flags on attachment: 267659

Committed r194310: <http://trac.webkit.org/changeset/194310>

All reviewed patches have been landed.  Closing bug.