Bug 216717

Summary: [JSC] PreciseAllocation's isNewlyAllocated flag should be propagated from isMarked at GC begin phase to make isLive correct
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch mark.lam: review+

Description Yusuke Suzuki 2020-09-18 15:04:32 PDT 
[JSC] PreciseAllocation's isNewlyAllocated flag should be propagated from isMarked at GC begin phase to make isLive correct
Comment 1 Yusuke Suzuki 2020-09-18 15:13:43 PDT 
Created attachment 409173 [details]
Patch
Comment 2 Yusuke Suzuki 2020-09-18 15:25:08 PDT 
Created attachment 409175 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2020-09-18 15:50:02 PDT 
<rdar://problem/69179885>
Comment 4 Mark Lam 2020-09-18 16:26:38 PDT 
Comment on attachment 409175 [details]
Patch

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

Nice.  Looks good so far but I still need to check this against the MarkedBlock code.  Here are some typos while I continue reviewing.

> Source/JavaScriptCore/ChangeLog:9
> +        However, this means that HeapCell::isLive will see this object dead until it is marked.

/object dead/object as dead/

> Source/JavaScriptCore/heap/PreciseAllocation.cpp:218
> +    // We do not need to care about concurrency here since marking thread is stopped right now. This is followin to the logic

/followin/equivalent/
Comment 5 Yusuke Suzuki 2020-09-18 16:31:18 PDT 
Comment on attachment 409175 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        However, this means that HeapCell::isLive will see this object dead until it is marked.
> 
> /object dead/object as dead/

Fixed.

>> Source/JavaScriptCore/heap/PreciseAllocation.cpp:218
>> +    // We do not need to care about concurrency here since marking thread is stopped right now. This is followin to the logic
> 
> /followin/equivalent/

Fixed.
Comment 6 Yusuke Suzuki 2020-09-18 16:32:30 PDT 
Created attachment 409178 [details]
Patch
Comment 7 Mark Lam 2020-09-18 17:36:35 PDT 
Comment on attachment 409178 [details]
Patch

r=me.  Nice fix, and nice comments documenting the reasoning behind all this.
Comment 8 Yusuke Suzuki 2020-09-18 18:22:49 PDT 
Committed r267304: <https://trac.webkit.org/changeset/267304>