Bug 209398

Created attachment 394212 [details]
Patch

Will be used in <rdar://problem/60541567>

Comment on attachment 394212 [details]
Patch

r=me.  It would have been nice if there‘s some way to share the implementation instead of having 2 copies, but I couldn’t think of a clean way to express that: probably more hassle than it’s worth.

(In reply to Mark Lam from comment #3)
> Comment on attachment 394212 [details]
> Patch
> 
> r=me.  It would have been nice if there‘s some way to share the
> implementation instead of having 2 copies, but I couldn’t think of a clean
> way to express that: probably more hassle than it’s worth.

Yeah, I was thinking about sharing implementation, but there is many conditions like CPU(ADDRESS64), USE(JSVALUE64) etc. And I think just writing this simple function here would be clear than sharing the implementation by putting a lot of ifdefs.

Committed r258824: <https://trac.webkit.org/changeset/258824>

<rdar://problem/60752174>

Comment on attachment 394212 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCJSValue.h:641
> +ALWAYS_INLINE void keepAlive(JSValue value)

This function does not seem to keep 'value' alive, so "keep alive" is not a great name for it.

It appears that you're trying to show the compiler that a given value is used at the point of the call. A better name for that might simply be "use" or "compilerUse".

Comment on attachment 394212 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSCJSValue.h:641
>> +ALWAYS_INLINE void keepAlive(JSValue value)
> 
> This function does not seem to keep 'value' alive, so "keep alive" is not a great name for it.
> 
> It appears that you're trying to show the compiler that a given value is used at the point of the call. A better name for that might simply be "use" or "compilerUse".

I think this "keepAlive" is keeping 'value' alive.
While the implementation of this is `compilerUse` or `use`, the effect and goal of this function is keeping 'value' alive by keeping it in conservative roots at the calling point.
So, this "keepAlive" name is describing the purpose and effect of this function, no?

Comment on attachment 394212 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSCJSValue.h:641
>>> +ALWAYS_INLINE void keepAlive(JSValue value)
>> 
>> This function does not seem to keep 'value' alive, so "keep alive" is not a great name for it.
>> 
>> It appears that you're trying to show the compiler that a given value is used at the point of the call. A better name for that might simply be "use" or "compilerUse".
> 
> I think this "keepAlive" is keeping 'value' alive.
> While the implementation of this is `compilerUse` or `use`, the effect and goal of this function is keeping 'value' alive by keeping it in conservative roots at the calling point.
> So, this "keepAlive" name is describing the purpose and effect of this function, no?

Note that FTL has the same feature. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp#L18347-L18355

Most confusing to me about the phrase "keepAlive" is when the function will stop keeping the value alive. 

If the goal is to create a root for conservative GC, maybe a better name is "gcRoot" or "conservativeRoot".

(In reply to Geoffrey Garen from comment #10)
> Most confusing to me about the phrase "keepAlive" is when the function will
> stop keeping the value alive. 
> 
> If the goal is to create a root for conservative GC, maybe a better name is
> "gcRoot" or "conservativeRoot".

I think `gcRoot` and `conservativeRoot` look ambiguous given that we already have `root` function in WebCore, and it is used for completely different purpose: returning opaque-root for the given pointer. Since "root" function is also used in GC's context, I think `gcRoot` / `conservativeRoot` look like the similar / same to `root` function in WebCore. But they are different.

How about using `keepAliveAtThisPoint` `ensureReachableFromGCRootAtThisPoint` instead? They clarify that this keep-alive continue until this point.

(In reply to Yusuke Suzuki from comment #11)
> (In reply to Geoffrey Garen from comment #10)
> > Most confusing to me about the phrase "keepAlive" is when the function will
> > stop keeping the value alive. 
> > 
> > If the goal is to create a root for conservative GC, maybe a better name is
> > "gcRoot" or "conservativeRoot".
> 
> I think `gcRoot` and `conservativeRoot` look ambiguous given that we already
> have `root` function in WebCore, and it is used for completely different
> purpose: returning opaque-root for the given pointer. Since "root" function
> is also used in GC's context, I think `gcRoot` / `conservativeRoot` look
> like the similar / same to `root` function in WebCore. But they are
> different.
> 
> How about using `keepAliveAtThisPoint`
> `ensureReachableFromGCRootAtThisPoint` instead? They clarify that this
> keep-alive continue until this point.

Example of root function in WebCore. https://github.com/WebKit/webkit/blob/master/Source/WebCore/bindings/js/JSCSSRuleCustom.h#L33-L42 This is not used for keeping value alive in conservative root. It is a function returning opaque-root for the given pointer.

(In reply to Yusuke Suzuki from comment #12)
> (In reply to Yusuke Suzuki from comment #11)
> > (In reply to Geoffrey Garen from comment #10)
> > > Most confusing to me about the phrase "keepAlive" is when the function will
> > > stop keeping the value alive. 
> > > 
> > > If the goal is to create a root for conservative GC, maybe a better name is
> > > "gcRoot" or "conservativeRoot".
> > 
> > I think `gcRoot` and `conservativeRoot` look ambiguous given that we already
> > have `root` function in WebCore, and it is used for completely different
> > purpose: returning opaque-root for the given pointer. Since "root" function
> > is also used in GC's context, I think `gcRoot` / `conservativeRoot` look
> > like the similar / same to `root` function in WebCore. But they are
> > different.
> > 
> > How about using `keepAliveAtThisPoint`
> > `ensureReachableFromGCRootAtThisPoint` instead? They clarify that this
> > keep-alive continue until this point.
> 
> Example of root function in WebCore.
> https://github.com/WebKit/webkit/blob/master/Source/WebCore/bindings/js/
> JSCSSRuleCustom.h#L33-L42 This is not used for keeping value alive in
> conservative root. It is a function returning opaque-root for the given
> pointer.

If user uses `root` function instead of `gcRoot` / `conservativeRoot` functions to keep value alive, this is wrong. And since both `root` and `gcRoot` will accept void*, it would be difficult to notice. So I think we should pick clear name here. Maybe, "ensureReachableFromGCRootAtThisPoint" would be the best.

(In reply to Yusuke Suzuki from comment #13)
> If user uses `root` function instead of `gcRoot` / `conservativeRoot`
> functions to keep value alive, this is wrong. And since both `root` and
> `gcRoot` will accept void*, it would be difficult to notice. So I think we
> should pick clear name here. Maybe, "ensureReachableFromGCRootAtThisPoint"
> would be the best.

How about something shorter like "ensureStillAliveHere"?  I like "ensure" better than "keep", and I think it is a better description of what this function is trying to do.

(In reply to Mark Lam from comment #14)
> (In reply to Yusuke Suzuki from comment #13)
> > If user uses `root` function instead of `gcRoot` / `conservativeRoot`
> > functions to keep value alive, this is wrong. And since both `root` and
> > `gcRoot` will accept void*, it would be difficult to notice. So I think we
> > should pick clear name here. Maybe, "ensureReachableFromGCRootAtThisPoint"
> > would be the best.
> 
> How about something shorter like "ensureStillAliveHere"?  I like "ensure"
> better than "keep", and I think it is a better description of what this
> function is trying to do.

Sounds fine! I'll rename it to ensureStillAliveHere.

Committed r258825: <https://trac.webkit.org/changeset/258825>

Nice! Way better name!

Comment on attachment 394212 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSCJSValue.h:641
>>>> +ALWAYS_INLINE void keepAlive(JSValue value)
>>> 
>>> This function does not seem to keep 'value' alive, so "keep alive" is not a great name for it.
>>> 
>>> It appears that you're trying to show the compiler that a given value is used at the point of the call. A better name for that might simply be "use" or "compilerUse".
>> 
>> I think this "keepAlive" is keeping 'value' alive.
>> While the implementation of this is `compilerUse` or `use`, the effect and goal of this function is keeping 'value' alive by keeping it in conservative roots at the calling point.
>> So, this "keepAlive" name is describing the purpose and effect of this function, no?
> 
> Note that FTL has the same feature. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp#L18347-L18355

I think it might have been clearer if this was a C++ class that does this when being destructed.