Bug 213345 – Unify Bitmap math loops in MarkedBlock::Handle::specializedSweep().

• Status: REOPENED
• Component: JavaScriptCore

[Mark Lam]

Also address feedback from Robin and Saam in https://bugs.webkit.org/show_bug.cgi?id=213071.

[Mark Lam]

Created attachment 402224 [details]
proposed patch.

[Mark Lam]

Created attachment 402225 [details]
proposed patch.

[Mark Lam]

Comment on attachment 402225 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:11
> +        understand than then old code.

/then/the/

> Source/JavaScriptCore/ChangeLog:38
> +        5. Aldo fixed some typos in comments.

/Aldo/Also/

[Robin Morisset]

Comment on attachment 402225 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/heap/MarkedBlockInlines.h:345
> +    if (emptyMode == NotEmpty) {

This section is *much* more readable than before, thanks!

[Saam Barati]

Comment on attachment 402225 [details]
proposed patch.

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

> Source/JavaScriptCore/heap/MarkedBlockInlines.h:369
> +    // At this point, a set bit in freeAtoms represents live cells.

this is no longer true, right? Opposite

[Saam Barati]

Comment on attachment 402225 [details]
proposed patch.

reinstating Robin's r+. I'm not done reviewing though

[Saam Barati]

Comment on attachment 402225 [details]
proposed patch.

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

> Source/JavaScriptCore/heap/MarkedBlockInlines.h:367
> +    bool isEmpty = !anyBits;

alternatively, you can just set this to false, then to true when iterating freeAtoms.forEachSetBit below

[Saam Barati]

Comment on attachment 402225 [details]
proposed patch.

r=me too

[Mark Lam]

Comment on attachment 402225 [details]
proposed patch.

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

>> Source/JavaScriptCore/heap/MarkedBlockInlines.h:367
>> +    bool isEmpty = !anyBits;
> 
> alternatively, you can just set this to false, then to true when iterating freeAtoms.forEachSetBit below

Good point. Thanks.

>> Source/JavaScriptCore/heap/MarkedBlockInlines.h:369
>> +    // At this point, a set bit in freeAtoms represents live cells.
> 
> this is no longer true, right? Opposite

Will fix.

[Mark Lam]

Created attachment 402261 [details]
patch for landing.

[Mark Lam]

Thanks for the reviews.  Landed in r263252: <http://trac.webkit.org/r263252>.

[Radar WebKit Bug Importer]

<rdar://problem/64516410>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 215312