Modify allocator_shim to support _aligned_alloc(), _aligned_free().

Modify allocator_shim to support _aligned_alloc(), _aligned_free().

As titled, pipes the Windows specific methods to the appropriate
internal allocators.

BUG=none
TEST=manual test with each allocator over various alignments.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149537

[DaleCurtis, 2012-07-27 18:32:25]

Jim, PTAL. This is the implementation on what you suggested via email.

I'm not sure what the proper way to go about writing tests here is. Just add an
entry to allocator_unittest? Then run with each ENV setting?

Thanks in advance!

[DaleCurtis, 2012-07-27 19:50:19]

+sgk for jemalloc change. Upstream has diverged significantly since the 2009
version of the code we're using, but it does have a je_memalign prototype like
I've added here.

Not sure what the best approach is here.  Should jemalloc be rolled? Does anyone
care?

[DaleCurtis, 2012-07-30 22:21:14]

jar: Ping? Should I find another reviewer?

[jar (doing other things), 2012-07-30 23:04:21]

http://codereview.chromium.org/10828054/diff/5/base/allocator/allocator_unitt...
File base/allocator/allocator_unittests.cc (right):

http://codereview.chromium.org/10828054/diff/5/base/allocator/allocator_unitt...
base/allocator/allocator_unittests.cc:487: for (int size = 1; size < 1048576;
size *= 2) {
By accident, tcmalloc will tend(?) to pass this test, as new (never before seen)
sizes are usually allocated out of a thread-local heap of a given "size class." 
Each time we try a new size, we will usually get the first element of a span,
which is a 4K aligned allocation).

It would probably be nicer if you would try for some larger alignment factors
(over 4K), and it would also be nicer if you asked for two aligned allocation of
each size (so that we'd be assured we weren't just getting the first item from a
class.

http://codereview.chromium.org/10828054/diff/5/base/allocator/allocator_unitt...
base/allocator/allocator_unittests.cc:490: _aligned_malloc(size,
kTestAlignments[i]));
I'd also suggest you allocate "size + 1" to avoid asking for powers of 2, which
*might* have a better shot an nice alignment IMO.

http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.cc
File base/allocator/win_allocator.cc (right):

http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.c...
base/allocator/win_allocator.cc:51: // Reserve enough space to ensure we can
align and set aligned_ptr[-1] to the
I think you may want to be cautious about args here.  I don't know who is
calling us, but we'll get in a lot of trouble if we call malloc with bogus
values.

Perhaps:

CHECK_GT(alignment, 0);  // perhaps, > 4 even???
CHECK_EQ(alignment & alignment - 1, 0);
CHECK_LT(alignment, 1024 * 1024 * 1024);

We also want to worry about wrapping.  Maybe:

size_t oversize = size + (alignment - 1) + sizeof(void*);
CHECK_LT(size, oversize);
CHECK_LT(alignment, oversize);

You could probably simplify some of those checks, but it is probably clearer
(and not performance critical) to leave them as explicit checks.

[DaleCurtis, 2012-07-31 00:44:22]

Thanks for the review! PTAL.

http://codereview.chromium.org/10828054/diff/5/base/allocator/allocator_unitt...
File base/allocator/allocator_unittests.cc (right):

http://codereview.chromium.org/10828054/diff/5/base/allocator/allocator_unitt...
base/allocator/allocator_unittests.cc:487: for (int size = 1; size < 1048576;
size *= 2) {
On 2012/07/30 23:04:22, jar wrote:
> By accident, tcmalloc will tend(?) to pass this test, as new (never before
seen)
> sizes are usually allocated out of a thread-local heap of a given "size
class." 
> Each time we try a new size, we will usually get the first element of a span,
> which is a 4K aligned allocation).
> 
> It would probably be nicer if you would try for some larger alignment factors
> (over 4K), and it would also be nicer if you asked for two aligned allocation
of
> each size (so that we'd be assured we weren't just getting the first item from
a
> class.

Added 8192, 16384 alignments and allocate two per pass.

http://codereview.chromium.org/10828054/diff/5/base/allocator/allocator_unitt...
base/allocator/allocator_unittests.cc:490: _aligned_malloc(size,
kTestAlignments[i]));
On 2012/07/30 23:04:22, jar wrote:
> I'd also suggest you allocate "size + 1" to avoid asking for powers of 2,
which
> *might* have a better shot an nice alignment IMO.

Switched to using NextSize() as many of the other tests here are using.  Tries
power_of_2-1, power_of_2, power_of_2+1.

http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.cc
File base/allocator/win_allocator.cc (right):

http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.c...
base/allocator/win_allocator.cc:51: // Reserve enough space to ensure we can
align and set aligned_ptr[-1] to the
On 2012/07/30 23:04:22, jar wrote:
> I think you may want to be cautious about args here.  I don't know who is
> calling us, but we'll get in a lot of trouble if we call malloc with bogus
> values.
> 
> Perhaps:
> 
> CHECK_GT(alignment, 0);  // perhaps, > 4 even???
> CHECK_EQ(alignment & alignment - 1, 0);
> CHECK_LT(alignment, 1024 * 1024 * 1024);
> 
> We also want to worry about wrapping.  Maybe:
> 
> size_t oversize = size + (alignment - 1) + sizeof(void*);
> CHECK_LT(size, oversize);
> CHECK_LT(alignment, oversize);
> 
> You could probably simplify some of those checks, but it is probably clearer
> (and not performance critical) to leave them as explicit checks.

Can you elaborate on the trouble we could face? From the MSDN docs on HeapAlloc,
it sounds like at worst we'll get a NULL ptr back:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa366597

I.e., no worse than a bogus call to win_heap_malloc(). Added some DCHECKs()
though for sanity. I've put most of these checks in allocator_shim though since
_aligned_malloc() guarantees its arguments are validated.

[jar (doing other things), 2012-07-31 03:18:51]

note: sgk is not working on Chrome anymore.

http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.cc
File base/allocator/win_allocator.cc (right):

http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.c...
base/allocator/win_allocator.cc:51: // Reserve enough space to ensure we can
align and set aligned_ptr[-1] to the
I didn't see where the args were being validated.  As a result, I was concerned
to be sure we do it here.

Example: Call for something that is 0.5GB aligned.
Ask for a total size 3.5GB.

We'll call malloc and ask for a total size of roughly 4 bytes, and well get an
allocation success <oops>.

We'll return to the caller a success, and the caller can then access memory
anywhere in the 3.5GB region that follows.

That is the class of problem I'd rather not see happen.

On 2012/07/31 00:44:22, DaleCurtis wrote:
> On 2012/07/30 23:04:22, jar wrote:
> > I think you may want to be cautious about args here.  I don't know who is
> > calling us, but we'll get in a lot of trouble if we call malloc with bogus
> > values.
> > 
> > Perhaps:
> > 
> > CHECK_GT(alignment, 0);  // perhaps, > 4 even???
> > CHECK_EQ(alignment & alignment - 1, 0);
> > CHECK_LT(alignment, 1024 * 1024 * 1024);
> > 
> > We also want to worry about wrapping.  Maybe:
> > 
> > size_t oversize = size + (alignment - 1) + sizeof(void*);
> > CHECK_LT(size, oversize);
> > CHECK_LT(alignment, oversize);
> > 
> > You could probably simplify some of those checks, but it is probably clearer
> > (and not performance critical) to leave them as explicit checks.
> 
> Can you elaborate on the trouble we could face? From the MSDN docs on
HeapAlloc,
> it sounds like at worst we'll get a NULL ptr back:
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa366597
> 
> I.e., no worse than a bogus call to win_heap_malloc(). Added some DCHECKs()
> though for sanity. I've put most of these checks in allocator_shim though
since
> _aligned_malloc() guarantees its arguments are validated.

[DaleCurtis, 2012-07-31 03:39:48]

-sgk then. Do you know if anyone is using JEMalloc now? I just pulled sgk from
git blame and git log is woefully short for it otherwise.

If no one's using it. Should we consider removing it?

http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.cc
File base/allocator/win_allocator.cc (right):

http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.c...
base/allocator/win_allocator.cc:51: // Reserve enough space to ensure we can
align and set aligned_ptr[-1] to the
On 2012/07/31 03:18:51, jar wrote:
> I didn't see where the args were being validated.  As a result, I was
concerned
> to be sure we do it here.
> 
> Example: Call for something that is 0.5GB aligned.
> Ask for a total size 3.5GB.
> 
> We'll call malloc and ask for a total size of roughly 4 bytes, and well get an
> allocation success <oops>.
> 
> We'll return to the caller a success, and the caller can then access memory
> anywhere in the 3.5GB region that follows.
> 
> That is the class of problem I'd rather not see happen.
> 
> On 2012/07/31 00:44:22, DaleCurtis wrote:
> > On 2012/07/30 23:04:22, jar wrote:
> > > I think you may want to be cautious about args here.  I don't know who is
> > > calling us, but we'll get in a lot of trouble if we call malloc with bogus
> > > values.
> > > 
> > > Perhaps:
> > > 
> > > CHECK_GT(alignment, 0);  // perhaps, > 4 even???
> > > CHECK_EQ(alignment & alignment - 1, 0);
> > > CHECK_LT(alignment, 1024 * 1024 * 1024);
> > > 
> > > We also want to worry about wrapping.  Maybe:
> > > 
> > > size_t oversize = size + (alignment - 1) + sizeof(void*);
> > > CHECK_LT(size, oversize);
> > > CHECK_LT(alignment, oversize);
> > > 
> > > You could probably simplify some of those checks, but it is probably
clearer
> > > (and not performance critical) to leave them as explicit checks.
> > 
> > Can you elaborate on the trouble we could face? From the MSDN docs on
> HeapAlloc,
> > it sounds like at worst we'll get a NULL ptr back:
> > http://msdn.microsoft.com/en-us/library/windows/desktop/aa366597
> > 
> > I.e., no worse than a bogus call to win_heap_malloc(). Added some DCHECKs()
> > though for sanity. I've put most of these checks in allocator_shim though
> since
> > _aligned_malloc() guarantees its arguments are validated.
> 

Gotcha, I thought there might be some weird HeapAlloc behavior I wasn't aware
of. The new patch set adds an overflow check.

Most users will probably be using base::AlignedAlloc which does input validation
as well:
http://git.chromium.org/gitweb/?p=chromium.git;a=blob;f=base/memory/aligned_m...

[jar (doing other things), 2012-08-01 21:33:51]

lgtm

[DaleCurtis, 2012-08-01 21:46:27]

On 2012/08/01 21:33:51, jar wrote:
> lgtm

Thanks Jim! CQ'ing.

[commit-bot: I haz the power, 2012-08-01 21:46:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10828054/6002

[commit-bot: I haz the power, 2012-08-01 21:46:50]

Failed to apply patch for third_party/jemalloc/README.chromium:
While running patch -p1 --forward --force;
patching file third_party/jemalloc/README.chromium
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
third_party/jemalloc/README.chromium.rej

[commit-bot: I haz the power, 2012-08-01 22:14:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10828054/9003

[commit-bot: I haz the power, 2012-08-01 22:14:56]

Failed to apply patch for third_party/jemalloc/chromium/jemalloc.c:
While running patch -p1 --forward --force;
patching file third_party/jemalloc/chromium/jemalloc.c
Hunk #1 FAILED at 6148.
1 out of 1 hunk FAILED -- saving rejects to file
third_party/jemalloc/chromium/jemalloc.c.rej

[DaleCurtis, 2012-08-01 22:32:25]

On 2012/08/01 22:14:56, I haz the power (commit-bot) wrote:
> Failed to apply patch for third_party/jemalloc/chromium/jemalloc.c:
> While running patch -p1 --forward --force;
> patching file third_party/jemalloc/chromium/jemalloc.c
> Hunk #1 FAILED at 6148.
> 1 out of 1 hunk FAILED -- saving rejects to file
> third_party/jemalloc/chromium/jemalloc.c.rej

Argh, CQ can't handle the CrLf line endings. I've landed the jemalloc. c change
manually:

http://src.chromium.org/viewvc/chrome?view=rev&revision=149516

Will CQ the rest shortly.

[commit-bot: I haz the power, 2012-08-01 22:33:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10828054/10013

[jar (doing other things), 2012-08-01 22:40:12]

re: je-malloc

Truth be known... we almost never use anything other than TCMalloc in the field.
 Developers link with Win allocator and dlls in house.

We wanted to have all allocators on hand, so that we could do perf tests at any
point.  At times, we've backed off to shipping the windows allocator (as an
experiment), but never gone back beyond a canary.

I'd strongly prefer (using my hording instinct) that we preserve support for
je-malloc if possible.  If it really gets too hard, then we'll punt... but this
example wasn't that bad IMO, and I'm glad you supported it.

On 2012/07/31 03:39:48, DaleCurtis wrote:
> -sgk then. Do you know if anyone is using JEMalloc now? I just pulled sgk from
> git blame and git log is woefully short for it otherwise.
> 
> If no one's using it. Should we consider removing it?
> 
> http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.cc
> File base/allocator/win_allocator.cc (right):
> 
>
http://codereview.chromium.org/10828054/diff/5/base/allocator/win_allocator.c...
> base/allocator/win_allocator.cc:51: // Reserve enough space to ensure we can
> align and set aligned_ptr[-1] to the
> On 2012/07/31 03:18:51, jar wrote:
> > I didn't see where the args were being validated.  As a result, I was
> concerned
> > to be sure we do it here.
> > 
> > Example: Call for something that is 0.5GB aligned.
> > Ask for a total size 3.5GB.
> > 
> > We'll call malloc and ask for a total size of roughly 4 bytes, and well get
an
> > allocation success <oops>.
> > 
> > We'll return to the caller a success, and the caller can then access memory
> > anywhere in the 3.5GB region that follows.
> > 
> > That is the class of problem I'd rather not see happen.
> > 
> > On 2012/07/31 00:44:22, DaleCurtis wrote:
> > > On 2012/07/30 23:04:22, jar wrote:
> > > > I think you may want to be cautious about args here.  I don't know who
is
> > > > calling us, but we'll get in a lot of trouble if we call malloc with
bogus
> > > > values.
> > > > 
> > > > Perhaps:
> > > > 
> > > > CHECK_GT(alignment, 0);  // perhaps, > 4 even???
> > > > CHECK_EQ(alignment & alignment - 1, 0);
> > > > CHECK_LT(alignment, 1024 * 1024 * 1024);
> > > > 
> > > > We also want to worry about wrapping.  Maybe:
> > > > 
> > > > size_t oversize = size + (alignment - 1) + sizeof(void*);
> > > > CHECK_LT(size, oversize);
> > > > CHECK_LT(alignment, oversize);
> > > > 
> > > > You could probably simplify some of those checks, but it is probably
> clearer
> > > > (and not performance critical) to leave them as explicit checks.
> > > 
> > > Can you elaborate on the trouble we could face? From the MSDN docs on
> > HeapAlloc,
> > > it sounds like at worst we'll get a NULL ptr back:
> > > http://msdn.microsoft.com/en-us/library/windows/desktop/aa366597
> > > 
> > > I.e., no worse than a bogus call to win_heap_malloc(). Added some
DCHECKs()
> > > though for sanity. I've put most of these checks in allocator_shim though
> > since
> > > _aligned_malloc() guarantees its arguments are validated.
> > 
> 
> Gotcha, I thought there might be some weird HeapAlloc behavior I wasn't aware
> of. The new patch set adds an overflow check.
> 
> Most users will probably be using base::AlignedAlloc which does input
validation
> as well:
>
http://git.chromium.org/gitweb/?p=chromium.git;a=blob;f=base/memory/aligned_m...

[commit-bot: I haz the power, 2012-08-02 00:00:24]

Change committed as 149537