| Summary: | Delete by val caching does not keep the subscript alive | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Justin Michaud <justin_michaud> | ||||||||
| Component: | JavaScriptCore | Assignee: | Justin Michaud <justin_michaud> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Justin Michaud
2020-02-28 14:33:27 PST
Created attachment 392021 [details]
Patch
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 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 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 ? Created attachment 392026 [details]
Patch
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 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. r=me too Created attachment 392169 [details]
Patch
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 on attachment 392169 [details] Patch Clearing flags on attachment: 392169 Committed r257728: <https://trac.webkit.org/changeset/257728> All reviewed patches have been landed. Closing bug. 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) |