Optimize test for pointer being in a partition on 32-bit.

Optimize test for pointer being in a partition on 32-bit.

We use a small (8KB) global bitmap.

R=tsepez@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158777

[cevans, 2013-10-02 16:37:32]

Here's that simple optimization we talked about.
In the future, we might want to move to 256KB superpages, which would further
reduce the size of the bitmap down to 2KB, if we care.

[Tom Sepez, 2013-10-02 16:51:31]

https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.cpp
File Source/wtf/PageAllocator.cpp (right):

https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.c...
Source/wtf/PageAllocator.cpp:181: size_t idx = raw >> (kSuperPageShift + 3);
same issue here with bit within byte index.

https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h
File Source/wtf/PageAllocator.h (right):

https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h...
Source/wtf/PageAllocator.h:92: class SuperPageBitmap {
Not sure this buys a lot over just having the s_bitmap in the partition alloc
class itself and doing the arithmetic there.  You've already got idefs sprinkled
liberally thoughout.

https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h...
Source/wtf/PageAllocator.h:98: size_t bit = raw & 7;
This doesn't seem right.  You want the low 3 bits of the shifted pointer to use
as the "bit within byte" index, not the low three bits of the pointer itself.

https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h...
Source/wtf/PageAllocator.h:106: static unsigned char s_bitmap[1 << (32 -
kSuperPageShift - 3)];
I'd like this as a constant so we can assert that idx is less than it in the
debug build.

[Tom Sepez, 2013-10-02 16:57:33]

https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h
File Source/wtf/PageAllocator.h (right):

https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h...
Source/wtf/PageAllocator.h:92: class SuperPageBitmap {
On 2013/10/02 16:51:31, Tom Sepez wrote:
> Not sure this buys a lot over just having the s_bitmap in the partition alloc
> class itself and doing the arithmetic there.  You've already got idefs
sprinkled
> liberally thoughout.

Alternatively, you could call this a SuperPageTracker, and put both the bitmap
and linked list implementations into it, ifdef'd appropriately.  Then you could
avoid having CPU() ifdefs outside of this code as it would abstract away the
differences.

[Chris Evans, 2013-10-02 19:40:14]

PTAL!

On 2013/10/02 16:51:31, Tom Sepez wrote:
>
https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.cpp
> File Source/wtf/PageAllocator.cpp (right):
> 
>
https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.c...
> Source/wtf/PageAllocator.cpp:181: size_t idx = raw >> (kSuperPageShift + 3);
> same issue here with bit within byte index.

Yeah nice catch, fixed?

> 
> https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h
> File Source/wtf/PageAllocator.h (right):
> 
>
https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h...
> Source/wtf/PageAllocator.h:92: class SuperPageBitmap {
> Not sure this buys a lot over just having the s_bitmap in the partition alloc
> class itself and doing the arithmetic there.  You've already got idefs
sprinkled
> liberally thoughout.

As discussed in person, moved all ifdefs out of PartitionAlloc.

> 
>
https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h...
> Source/wtf/PageAllocator.h:98: size_t bit = raw & 7;
> This doesn't seem right.  You want the low 3 bits of the shifted pointer to
use
> as the "bit within byte" index, not the low three bits of the pointer itself.

Yep yep.

> 
>
https://codereview.chromium.org/25640003/diff/5001/Source/wtf/PageAllocator.h...
> Source/wtf/PageAllocator.h:106: static unsigned char s_bitmap[1 << (32 -
> kSuperPageShift - 3)];
> I'd like this as a constant so we can assert that idx is less than it in the
> debug build.

Added ASSERT using sizeof. Useful ASSERT I agree.

[Tom Sepez, 2013-10-02 22:56:52]

Still some ifdefs that might go away.

https://codereview.chromium.org/25640003/diff/15001/Source/wtf/PageAllocator.cpp
File Source/wtf/PageAllocator.cpp (right):

https://codereview.chromium.org/25640003/diff/15001/Source/wtf/PageAllocator....
Source/wtf/PageAllocator.cpp:100: SuperPageBitmap::registerSuperPage(ret);
I think you can get rid of these ifdefs as well by making their bodies empty
(below) and moving to the header file.

[Chris Evans, 2013-10-02 23:37:11]

On 2013/10/02 22:56:52, Tom Sepez wrote:
> Still some ifdefs that might go away.
> 
>
https://codereview.chromium.org/25640003/diff/15001/Source/wtf/PageAllocator.cpp
> File Source/wtf/PageAllocator.cpp (right):
> 
>
https://codereview.chromium.org/25640003/diff/15001/Source/wtf/PageAllocator....
> Source/wtf/PageAllocator.cpp:100: SuperPageBitmap::registerSuperPage(ret);
> I think you can get rid of these ifdefs as well by making their bodies empty
> (below) and moving to the header file.

Yeah. PTAL?

[Tom Sepez, 2013-10-02 23:39:12]

lgtm

[ojan, 2013-10-03 00:44:09]

https://codereview.chromium.org/25640003/diff/2001/Source/wtf/PageAllocator.cpp
File Source/wtf/PageAllocator.cpp (right):

https://codereview.chromium.org/25640003/diff/2001/Source/wtf/PageAllocator.c...
Source/wtf/PageAllocator.cpp:176: size_t idx = raw >> 3;
Nit: Here an elsewhere, I'm not sure what "idx" stands for. Is this supposed to
be "index"? Blink code prefers to avoid abbreviations in general.

[Chris Evans, 2013-10-03 07:09:33]

Committed patchset #5 manually as r158777 (presubmit successful).

[cevans, 2013-10-03 07:13:26]

On 2013/10/03 00:44:09, ojan wrote:
>
https://codereview.chromium.org/25640003/diff/2001/Source/wtf/PageAllocator.cpp
> File Source/wtf/PageAllocator.cpp (right):
> 
>
https://codereview.chromium.org/25640003/diff/2001/Source/wtf/PageAllocator.c...
> Source/wtf/PageAllocator.cpp:176: size_t idx = raw >> 3;
> Nit: Here an elsewhere, I'm not sure what "idx" stands for. Is this supposed
to
> be "index"? Blink code prefers to avoid abbreviations in general.

Thanks, this was fixed for landing.