Bug 213345 - Unify Bitmap math loops in MarkedBlock::Handle::specializedSweep().
Summary: Unify Bitmap math loops in MarkedBlock::Handle::specializedSweep().
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 215312
Blocks:
  Show dependency tree / graph
 
Reported: 2020-06-18 11:40 PDT by Mark Lam
Modified: 2020-08-09 02:07 PDT (History)
11 users (show)

See Also:

Attachments
proposed patch. (28.21 KB, patch)
2020-06-18 12:10 PDT, Mark Lam 		no flags Details | Formatted Diff | Diff
proposed patch. (28.10 KB, patch)
2020-06-18 12:19 PDT, Mark Lam 		saam: review+
Details | Formatted Diff | Diff
patch for landing. (29.20 KB, patch)
2020-06-18 19:14 PDT, Mark Lam 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-06-18 11:40:23 PDT 
Also address feedback from Robin and Saam in https://bugs.webkit.org/show_bug.cgi?id=213071.
Comment 1 Mark Lam 2020-06-18 12:10:40 PDT 
Created attachment 402224 [details]
proposed patch.
Comment 2 Mark Lam 2020-06-18 12:19:08 PDT 
Created attachment 402225 [details]
proposed patch.
Comment 3 Mark Lam 2020-06-18 12:27:05 PDT 
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/
Comment 4 Robin Morisset 2020-06-18 12:41:15 PDT 
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!
Comment 5 Saam Barati 2020-06-18 12:47:04 PDT 
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
Comment 6 Saam Barati 2020-06-18 12:47:31 PDT 
Comment on attachment 402225 [details]
proposed patch.

reinstating Robin's r+. I'm not done reviewing though
Comment 7 Saam Barati 2020-06-18 12:52:47 PDT 
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
Comment 8 Saam Barati 2020-06-18 12:55:03 PDT 
Comment on attachment 402225 [details]
proposed patch.

r=me too
Comment 9 Mark Lam 2020-06-18 13:07:54 PDT 
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.
Comment 10 Mark Lam 2020-06-18 19:14:17 PDT 
Created attachment 402261 [details]
patch for landing.
Comment 11 Mark Lam 2020-06-18 20:31:20 PDT 
Thanks for the reviews.  Landed in r263252: <http://trac.webkit.org/r263252>.
Comment 12 Radar WebKit Bug Importer 2020-06-18 20:32:17 PDT 
<rdar://problem/64516410>
Comment 13 WebKit Commit Bot 2020-08-09 02:07:01 PDT 
Re-opened since this is blocked by bug 215312