Bug 209398 – [JSC] Add JSC::keepAlive(JSValue)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Add JSC::keepAlive(JSValue)

[Yusuke Suzuki]

Created attachment 394212 [details]
Patch

[Yusuke Suzuki]

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

[Mark Lam]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

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

[Radar WebKit Bug Importer]

<rdar://problem/60752174>

[Geoffrey Garen]

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".

[Yusuke Suzuki]

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?

[Yusuke Suzuki]

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

[Geoffrey Garen]

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".

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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.

[Mark Lam]

(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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

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

[Darin Adler]

Nice! Way better name!

[Keith Miller]

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.