Bug 212190 - Add more Bitmap methods.
Summary: Add more Bitmap methods.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-20 23:56 PDT by Mark Lam
Modified: 2020-05-21 15:14 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (23.57 KB, patch)
2020-05-21 00:02 PDT, Mark Lam
rmorisset: review+
Details | Formatted Diff | Diff
patch for landing. (16.48 KB, patch)
2020-05-21 14:36 PDT, Mark Lam
rmorisset: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-05-20 23:56:27 PDT
Specifically,
    setEachNthBit - sets every Nth bit starting at a specified start bit
    operator=     - assignment
    operator|=    - bit or and assignment
    operator&=    - bit and and assignment
    operator^=    - bit xor and assignment
Comment 1 Radar WebKit Bug Importer 2020-05-20 23:56:56 PDT
<rdar://problem/63481333>
Comment 2 Mark Lam 2020-05-21 00:02:26 PDT
Created attachment 399939 [details]
proposed patch.
Comment 3 Sam Weinig 2020-05-21 10:54:00 PDT
What do you plan to do with these?
Comment 4 Mark Lam 2020-05-21 11:01:04 PDT
(In reply to Sam Weinig from comment #3)
> What do you plan to do with these?

I'm exploring implementing a MarkedBlock FreeList using Bitmap instead of a linked list of FreeCells.  For that purpose, computation of which block atoms are free is done using bit math on Marked bits, NewlyAllocated bits, and some other mask.  The setEachNthBit() method is needed to make the other mask.
Comment 5 Robin Morisset 2020-05-21 11:45:46 PDT
Comment on attachment 399939 [details]
proposed patch.

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

r=me, with a couple of suggestions for the tests.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1001
> +    {

Is this block just the same as the one above except for s/size/smallsize/g ?
If so, is it possible to make it an helper function to avoid the repetition?

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1093
> +                temp.set(i, (randomNumber() > 0.5));

I am not sure that a random number in a test is a great idea. It sounds like a recipe for flakiness/non-reproducibility.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1102
> +    Bitmap<smallSize, WordType> smallTemp;

Same comment as above, maybe try merging the tests for smallSize and for size with a helper function.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1113
> +                smallTemp.set(i, (randomNumber() > 0.5));

see my comment above, I think it might be nicer to bake somewhere (at the top of the file?) an array with the random-looking values, instead of drawing them truly randomly at each execution.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1427
> +        // 1 & 0

typo: & instead of ^

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1434
> +        // 1 & 1

ditto.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1506
> +        // 1 & 0

ditto.

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1513
> +        // 1 & 1

ditto.
Comment 6 Mark Lam 2020-05-21 12:44:06 PDT
Comment on attachment 399939 [details]
proposed patch.

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

>> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1001
>> +    {
> 
> Is this block just the same as the one above except for s/size/smallsize/g ?
> If so, is it possible to make it an helper function to avoid the repetition?

Not identical because of the different data sets used.  Also I don't have predetermined data sets for the smallSize case.  So, I'm using randomNumber to generate them in some of the cases below where needed.  I can look into refactoring this.

>> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1427
>> +        // 1 & 0
> 
> typo: & instead of ^

Thanks fixed.
Comment 7 Mark Lam 2020-05-21 14:36:32 PDT
Created attachment 399981 [details]
patch for landing.
Comment 8 Robin Morisset 2020-05-21 14:43:21 PDT
Comment on attachment 399981 [details]
patch for landing.

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

Thanks for improving the tests!

> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:29
> +#include <wtf/RandomNumber.h>

No longer needed.
Comment 9 Mark Lam 2020-05-21 14:44:33 PDT
Comment on attachment 399981 [details]
patch for landing.

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

>> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:29
>> +#include <wtf/RandomNumber.h>
> 
> No longer needed.

Will remove.
Comment 10 Mark Lam 2020-05-21 15:14:11 PDT
Thanks for the reviews.  Landed in r262030: <http://trac.webkit.org/r262030>.