Bug 208393 – Delete by val caching does not keep the subscript alive

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Michaud]

Delete by val caching does not keep the subscript alive. It should use cacheable identifier to do this.

[Justin Michaud]

Created attachment 392021 [details]
Patch

[Saam Barati]

Comment on attachment 392021 [details]
Patch

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

> Source/JavaScriptCore/runtime/CacheableIdentifier.h:50
> +    enum DoNotKeepAliveTag { DoNotKeepAlive };

This isn't the right name. We definitely want both to be "kept" alive. But, this is just to indicate that it's a non-cell pointer we don't need to visit. Not sure what the right name is, but "DoNotKeepAlive" is super misleading.

Maybe something like "OwnedByCodeBlock"?

[Yusuke Suzuki]

Comment on attachment 392021 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CacheableIdentifier.h:50
>> +    enum DoNotKeepAliveTag { DoNotKeepAlive };
> 
> This isn't the right name. We definitely want both to be "kept" alive. But, this is just to indicate that it's a non-cell pointer we don't need to visit. Not sure what the right name is, but "DoNotKeepAlive" is super misleading.
> 
> Maybe something like "OwnedByCodeBlock"?

How about defining static factory functions? Like,

CacheableIdentifier::createWithoutKeepingAlive(const Identifier&), and hiding the constructor.

[Yusuke Suzuki]

Comment on attachment 392021 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/CacheableIdentifier.h:50
>>> +    enum DoNotKeepAliveTag { DoNotKeepAlive };
>> 
>> This isn't the right name. We definitely want both to be "kept" alive. But, this is just to indicate that it's a non-cell pointer we don't need to visit. Not sure what the right name is, but "DoNotKeepAlive" is super misleading.
>> 
>> Maybe something like "OwnedByCodeBlock"?
> 
> How about defining static factory functions? Like,
> 
> CacheableIdentifier::createWithoutKeepingAlive(const Identifier&), and hiding the constructor.

CacheableIdentifier::createFromIdentifierOwnedByCodeBlock ?

[Justin Michaud]

Created attachment 392026 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 392026 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/CacheableIdentifier.h:50
>      inline CacheableIdentifier(JSCell* identifier);

Personally I like making this constructor hidden too, and defining "createFromCell(JSCell*)` factory function instead, but it's up to you :).

[Saam Barati]

Comment on attachment 392026 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CacheableIdentifier.h:50
>>      inline CacheableIdentifier(JSCell* identifier);
> 
> Personally I like making this constructor hidden too, and defining "createFromCell(JSCell*)` factory function instead, but it's up to you :).

+1

It's weird to have one path use constructor, and one use the static factory thingy.

[Saam Barati]

r=me too

[Justin Michaud]

rdar://59933630

[Justin Michaud]

Created attachment 392169 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 392169 [details]:

editing/spelling/spellcheck-paste-continuous-disabled.html bug 208016 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 392169 [details]
Patch

Clearing flags on attachment: 392169

Committed r257728: <https://trac.webkit.org/changeset/257728>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 392169 [details]
Patch

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

> Source/JavaScriptCore/runtime/CacheableIdentifier.h:87
> +    explicit inline CacheableIdentifier(const Identifier&);

why make this explicit but not "createFromIdentifierOwnedByCodeBlock" since that's the API of this class (since these are private ctors)