Bug 208393

Summary: Delete by val caching does not keep the subscript alive
Product: WebKit Reporter: Justin Michaud <justin_michaud>
Component: JavaScriptCoreAssignee: Justin Michaud <justin_michaud>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Justin Michaud 2020-02-28 14:33:27 PST
Delete by val caching does not keep the subscript alive. It should use cacheable identifier to do this.
Comment 1 Justin Michaud 2020-02-28 14:37:52 PST
Created attachment 392021 [details]
Patch
Comment 2 Saam Barati 2020-02-28 14:40:59 PST
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"?
Comment 3 Yusuke Suzuki 2020-02-28 15:23:47 PST
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.
Comment 4 Yusuke Suzuki 2020-02-28 15:24:33 PST
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 ?
Comment 5 Justin Michaud 2020-02-28 15:39:13 PST
Created attachment 392026 [details]
Patch
Comment 6 Yusuke Suzuki 2020-02-28 15:59:40 PST
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 :).
Comment 7 Saam Barati 2020-02-28 18:13:08 PST
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.
Comment 8 Saam Barati 2020-02-28 18:13:41 PST
r=me too
Comment 9 Justin Michaud 2020-03-02 11:18:13 PST
rdar://59933630
Comment 10 Justin Michaud 2020-03-02 11:57:06 PST
Created attachment 392169 [details]
Patch
Comment 11 WebKit Commit Bot 2020-03-02 12:57:51 PST
The commit-queue encountered the following flaky tests while processing attachment 392169 [details]:

editing/spelling/spellcheck-paste-continuous-disabled.html bug 208016 (authors: g.czajkowski@samsung.com and mark.lam@apple.com)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2020-03-02 12:58:26 PST
Comment on attachment 392169 [details]
Patch

Clearing flags on attachment: 392169

Committed r257728: <https://trac.webkit.org/changeset/257728>
Comment 13 WebKit Commit Bot 2020-03-02 12:58:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Saam Barati 2020-03-02 17:23:13 PST
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)