Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(246)

Issue 21666003: Enhancements to PartitionAlloc to support threading and arbitrary allocation sizes. (Closed)

Created:
7 years, 4 months ago by Chris Evans
Modified:
7 years, 4 months ago
CC:
blink-reviews, loislo+blink_chromium.org, eae+blinkwatch, leviw+renderwatch, yurys+blink_chromium.org, dglazkov+blink, adamk+blink_chromium.org, jchaffraix+rendering, jeez
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 1

Patch Set 2 : Review items. #

Total comments: 3

Patch Set 3 : Review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+254 lines, -56 lines) Patch
M Source/core/dom/Node.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderObject.cpp View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/wtf/Atomics.h View 1 1 chunk +10 lines, -10 lines 0 comments Download
M Source/wtf/PartitionAlloc.h View 1 2 9 chunks +72 lines, -15 lines 0 comments Download
M Source/wtf/PartitionAlloc.cpp View 2 chunks +31 lines, -0 lines 0 comments Download
M Source/wtf/PartitionAllocTest.cpp View 1 chunk +79 lines, -0 lines 0 comments Download
A + Source/wtf/SpinLock.h View 1 2 1 chunk +15 lines, -12 lines 0 comments Download
A + Source/wtf/SpinLockTest.cpp View 1 2 1 chunk +41 lines, -19 lines 0 comments Download
M Source/wtf/wtf.gypi View 1 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
Chris Evans
7 years, 4 months ago (2013-08-02 23:35:07 UTC) #1
abarth-chromium
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#newcode246 Source/wtf/PartitionAlloc.h:246: atomicDecrement(&root->lock); Is this better than TCMalloc_SpinLock? In any case, ...
7 years, 4 months ago (2013-08-02 23:58:04 UTC) #2
Tom Sepez
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#newcode246 > ...
7 years, 4 months ago (2013-08-03 00:12:49 UTC) #3
Chris Evans
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 ...
7 years, 4 months ago (2013-08-05 18:43:56 UTC) #4
Chris Evans
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#newcode246 > ...
7 years, 4 months ago (2013-08-05 18:50:21 UTC) #5
Tom Sepez
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.h#newcode45 Source/wtf/PartitionAlloc.h:45: // Allocations must not exceed a ...
7 years, 4 months ago (2013-08-05 19:02:36 UTC) #6
abarth-chromium
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#newcode34 Source/wtf/SpinLock.h:34: // DESCRIPTION "DESCRIPTION" ? https://codereview.chromium.org/21666003/diff/6001/Source/wtf/SpinLockTest.cpp File Source/wtf/SpinLockTest.cpp (right): ...
7 years, 4 months ago (2013-08-05 20:40:25 UTC) #7
Chris Evans
Committed patchset #3 manually as r155544 (presubmit successful).
7 years, 4 months ago (2013-08-05 22:20:03 UTC) #8
abarth-chromium
7 years, 4 months ago (2013-08-05 22:37:57 UTC) #9
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698