Enhancements to PartitionAlloc to support threading and arbitrary allocation sizes.

Enhancements to PartitionAlloc to support threading and arbitrary allocation sizes.

BUG=267094
TEST=WTF_PartitionAlloc.GenericAlloc
R=abarth@chromium.org, tsepez@chromium.org

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

[abarth-chromium, 2013-08-02 23:58:04]

https://codereview.chromium.org/21666003/diff/1/Source/wtf/PartitionAlloc.h
File Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/21666003/diff/1/Source/wtf/PartitionAlloc.h#n...
Source/wtf/PartitionAlloc.h:246: atomicDecrement(&root->lock);
Is this better than TCMalloc_SpinLock?

In any case, it's a more general primitive that we should factor out into its
own header.

You should test this on Mac, where atomicIncrement is stupidly expensive.

[Tom Sepez, 2013-08-03 00:12:49]

On 2013/08/02 23:58:04, abarth wrote:
> https://codereview.chromium.org/21666003/diff/1/Source/wtf/PartitionAlloc.h
> File Source/wtf/PartitionAlloc.h (right):
> 
>
https://codereview.chromium.org/21666003/diff/1/Source/wtf/PartitionAlloc.h#n...
> Source/wtf/PartitionAlloc.h:246: atomicDecrement(&root->lock);
> Is this better than TCMalloc_SpinLock?
> 
> In any case, it's a more general primitive that we should factor out into its
> own header.
> 
with its own unit test to make sure it works. But that's work.

[Chris Evans, 2013-08-05 18:43:56]

On 2013/08/03 00:12:49, Tom Sepez wrote:
> On 2013/08/02 23:58:04, abarth wrote:
> > https://codereview.chromium.org/21666003/diff/1/Source/wtf/PartitionAlloc.h
> > File Source/wtf/PartitionAlloc.h (right):
> > 
> >
>
https://codereview.chromium.org/21666003/diff/1/Source/wtf/PartitionAlloc.h#n...
> > Source/wtf/PartitionAlloc.h:246: atomicDecrement(&root->lock);
> > Is this better than TCMalloc_SpinLock?
> > 
> > In any case, it's a more general primitive that we should factor out into
its
> > own header.
> > 
> with its own unit test to make sure it works. But that's work.

... but it's work worth doing, so see SpinLockTest.cpp in the latest uploaded
CL. Note that it fails egregiously and non-deterministically without the
locking.

PTAL!

[Chris Evans, 2013-08-05 18:50:21]

On 2013/08/02 23:58:04, abarth wrote:
> https://codereview.chromium.org/21666003/diff/1/Source/wtf/PartitionAlloc.h
> File Source/wtf/PartitionAlloc.h (right):
> 
>
https://codereview.chromium.org/21666003/diff/1/Source/wtf/PartitionAlloc.h#n...
> Source/wtf/PartitionAlloc.h:246: atomicDecrement(&root->lock);
> Is this better than TCMalloc_SpinLock?
> 
> In any case, it's a more general primitive that we should factor out into its
> own header.

Done: SpinLock.h

If you look at TCMalloc_SpinLock, it (bizarrely?) seems to be based on a mutex.
This isn't what we want here.
Note that I decided to stick with raw lock / unlock functions instead of a
"locker" class because I'm not confident it makes sense to have a pseudo-copy of
the lock pointer as a member variable. I prefer to keep it simple for now.

> 
> You should test this on Mac, where atomicIncrement is stupidly expensive.

Do you have a reference? atomicIncrement() should only be a function of
{compiler,CPU}, not platform. The implementation uses a compiler intrinsic, for
which the only likely implementation on Intel would be a platform-independent
bus-locked add instruction.

That said, I've arranged for the test to take about 100ms on Linux, so we can
look at some relative timings as a basic sanity check. I've further checked that
the addition of locking doesn't result in a problematic performance on Linux,
I'll add details and timings of that testing to the bug.

PTAL!

[Tom Sepez, 2013-08-05 19:02:36]

LGTM with nit.

https://codereview.chromium.org/21666003/diff/6001/Source/wtf/PartitionAlloc.h
File Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/21666003/diff/6001/Source/wtf/PartitionAlloc....
Source/wtf/PartitionAlloc.h:45: // Allocations must not exceed a max size,
typically 4088 at this time.
nit: 4088 bytes

[abarth-chromium, 2013-08-05 20:40:25]

lgtm

https://codereview.chromium.org/21666003/diff/6001/Source/wtf/SpinLock.h
File Source/wtf/SpinLock.h (right):

https://codereview.chromium.org/21666003/diff/6001/Source/wtf/SpinLock.h#newc...
Source/wtf/SpinLock.h:34: // DESCRIPTION
"DESCRIPTION" ?

https://codereview.chromium.org/21666003/diff/6001/Source/wtf/SpinLockTest.cpp
File Source/wtf/SpinLockTest.cpp (right):

https://codereview.chromium.org/21666003/diff/6001/Source/wtf/SpinLockTest.cp...
Source/wtf/SpinLockTest.cpp:43: static void bufFill(volatile char* buf, char
fillPattern)
bufFill -> fillBuffer?
bug -> buffer?

Blink's style is generally to use complete words in variable names.

[Chris Evans, 2013-08-05 22:20:03]

Committed patchset #3 manually as r155544 (presubmit successful).

[abarth-chromium, 2013-08-05 22:37:57]

On 2013/08/05 18:50:21, Chris Evans wrote:
> > You should test this on Mac, where atomicIncrement is stupidly expensive.
> 
> Do you have a reference? atomicIncrement() should only be a function of
> {compiler,CPU}, not platform. The implementation uses a compiler intrinsic,
for
> which the only likely implementation on Intel would be a platform-independent
> bus-locked add instruction.

I was thinking about NoBarrier_AtomicIncrement from atomicops_internals_mac.h:

http://src.chromium.org/viewvc/chrome/trunk/src/base/atomicops_internals_mac.h

That implementation calls OSAtomicAdd32, which is an indirect jump in a library.
 This version looks to call a compiler intrinsic.