Bug 212190 – Add more Bitmap methods.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Mark Lam]

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

[Radar WebKit Bug Importer]

<rdar://problem/63481333>

[Mark Lam]

Created attachment 399939 [details]
proposed patch.

[Sam Weinig]

What do you plan to do with these?

[Mark Lam]

(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.

[Robin Morisset]

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.

[Mark Lam]

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.

[Mark Lam]

Created attachment 399981 [details]
patch for landing.

[Robin Morisset]

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.

[Mark Lam]

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.

[Mark Lam]

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

[Michael Catanzaro]

This introduced a nice GCC warning spam:

In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f0a787a9-2.cpp:5:
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:194:16: warning: implicitly-declared ‘constexpr WTF::Bitmap<4>::Bitmap(const WTF::Bitmap<4>&)’ is deprecated [-Wdeprecated-copy]
  194 |         return result;
      |                ^~~~~~

because:

DerivedSources/ForwardingHeaders/wtf/Bitmap.h:480:13: note: because ‘WTF::Bitmap<4>’ has user-provided ‘void WTF::Bitmap<bitmapSize, WordType>::operator=(const WTF::Bitmap<bitmapSize, WordType>&) [with long unsigned int bitmapSize = 4; WordType = unsigned int]’
  480 | inline void Bitmap<bitmapSize, WordType>::operator=(const Bitmap& other)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

To avoid the warning, we should either (a) remove the copy assignment operator, or (b) add a copy constructor. Either way would be fine, but in this case I'd prefer (a) because I don't think the new copy assignment operator is needed. It uses a loop to manually copy the bits of the sole member variable, a std::array, but a default copy of the array would be equivalent, right?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #11)
> This introduced a nice GCC warning spam:

Created: bug #212421