Actually on Windows VirtualAlloc'ated address might have bigger alignment tha...

Actually on Windows VirtualAlloc'ated address might have bigger alignment than reported by SYSTEM_INFO.dwPageSize, see SYSTEM_INFO.dwAllocationGranularity.

As coalescing of spans in tcmalloc requires that those spans are physically adjacent, using smaller alignment inhibited coalescing.  Additional benefit is we now
require ways less memory at least in some case.

BUG=22701
Committed from issue: http://codereview.chromium.org/222028 at rev http://src.chromium.org/viewvc/chrome?view=rev&revision=27162

[sgk, 2009-09-24 17:08:48]

two style nits

(Code seems right to me, but others should weigh in.)

http://codereview.chromium.org/235003/diff/1/2
File third_party/tcmalloc/port.cc (right):

http://codereview.chromium.org/235003/diff/1/2#newcode74
Line 74: pagesize = std::max(system_info.dwPageSize,
system_info.dwAllocationGranularity);
80 chars

http://codereview.chromium.org/235003/diff/1/2#newcode198
Line 198: // TODO(antonm): proper processing of alignments in actual_size and
decommitting.
80 chars

[antonm, 2009-09-24 17:19:59]

Thanks a lot for review, Steven.

http://codereview.chromium.org/235003/diff/1/2
File third_party/tcmalloc/port.cc (right):

http://codereview.chromium.org/235003/diff/1/2#newcode74
Line 74: pagesize = std::max(system_info.dwPageSize,
system_info.dwAllocationGranularity);
On 2009/09/24 17:08:48, sgk wrote:
> 80 chars

Done.

http://codereview.chromium.org/235003/diff/1/2#newcode198
Line 198: // TODO(antonm): proper processing of alignments in actual_size and
decommitting.
On 2009/09/24 17:08:48, sgk wrote:
> 80 chars

Done.

[jamesr, 2009-09-24 18:21:52]

On 2009/09/24 17:19:59, antonm wrote:
> Thanks a lot for review, Steven.
> 
> http://codereview.chromium.org/235003/diff/1/2
> File third_party/tcmalloc/port.cc (right):
> 
> http://codereview.chromium.org/235003/diff/1/2#newcode74
> Line 74: pagesize = std::max(system_info.dwPageSize,
> system_info.dwAllocationGranularity);
> On 2009/09/24 17:08:48, sgk wrote:
> > 80 chars
> 
> Done.
> 
> http://codereview.chromium.org/235003/diff/1/2#newcode198
> Line 198: // TODO(antonm): proper processing of alignments in actual_size and
> decommitting.
> On 2009/09/24 17:08:48, sgk wrote:
> > 80 chars
> 
> Done.

Looks good to me.  I tested the theory by doing allocations of 17 pages (65k)
increasing in increments of 16 pages (64k).  The dwAllocationGranularity on my
machine is 16 pages and I observed a 15 page gap between each allocation.  I ran
out of address space after 239 allocations.

As jar mentioned on the bug, I think it's also worthwhile to modify the 2-level
page map to avoid fragmentation there as well.

[jamesr, 2009-09-24 20:38:12]

Since Anton is not a Chromium committer I plan to run this patch through the
trybots and commit later today.  Manual testing shows that this significantly
helps with the reduced testcase (I can now run up to having two 358MB
allocations live before running out of virtual address space compared to around
35MB before) and on one of the websites where this problem was initially
reported (the site loads with about 170MB of memory used instead of 1.3GB+).

Are there any objections?

[jar (doing other things), 2009-09-24 21:17:18]

http://codereview.chromium.org/235003/diff/1/2
File third_party/tcmalloc/port.cc (right):

http://codereview.chromium.org/235003/diff/1/2#newcode209
Line 209: void* result = VirtualAlloc(0, size + extra,
Note that the requested size potentially includes extra.

As a result, I would expect that it would be better to inform the caller that
the actual_size is the total requested size, including extra.

What am I missing?

[antonm, 2009-09-24 21:43:44]

Jim, thanks a lot for comments!

http://codereview.chromium.org/235003/diff/1/2
File third_party/tcmalloc/port.cc (right):

http://codereview.chromium.org/235003/diff/1/2#newcode209
Line 209: void* result = VirtualAlloc(0, size + extra,
On 2009/09/24 21:17:18, jar wrote:
> Note that the requested size potentially includes extra.
> 
> As a result, I would expect that it would be better to inform the caller that
> the actual_size is the total requested size, including extra.
> 
> What am I missing?

I put a TODO instead of fixing alignment stuff for one reason: it's kind of
orthogonal to the problem I try to solve :)

And it is apparently not at all easy.  For example, it's not only a matter of
adding extra, somehow I need to communicate back original pointer, before adding
adjust, otherwise coalescing with the previous span would suffer, correct?  And
release could should be adjusted as well.

Luckily, I would hope tcmalloc is never or rarely invoked with alignment >
pagesize.  And if this change goes in, it'd require > 64K alignment to expose
the bug.

So I thought to postpone fixing those issues.  Was it a reasonable choice?

[jamesr, 2009-09-24 21:56:48]

On 2009/09/24 21:43:44, antonm wrote:
> Jim, thanks a lot for comments!
> 
> http://codereview.chromium.org/235003/diff/1/2
> File third_party/tcmalloc/port.cc (right):
> 
> http://codereview.chromium.org/235003/diff/1/2#newcode209
> Line 209: void* result = VirtualAlloc(0, size + extra,
> On 2009/09/24 21:17:18, jar wrote:
> > Note that the requested size potentially includes extra.
> > 
> > As a result, I would expect that it would be better to inform the caller
that
> > the actual_size is the total requested size, including extra.
> > 
> > What am I missing?
> 
> I put a TODO instead of fixing alignment stuff for one reason: it's kind of
> orthogonal to the problem I try to solve :)
> 
> And it is apparently not at all easy.  For example, it's not only a matter of
> adding extra, somehow I need to communicate back original pointer, before
adding
> adjust, otherwise coalescing with the previous span would suffer, correct? 
And
> release could should be adjusted as well.
> 
> Luckily, I would hope tcmalloc is never or rarely invoked with alignment >
> pagesize.  And if this change goes in, it'd require > 64K alignment to expose
> the bug.
> 
> So I thought to postpone fixing those issues.  Was it a reasonable choice?

There are no callers currently with odd alignment requirements - in the Chromium
codebase today all callers either use the default argument alignment=0 (which is
floored up to pagesize) or they use kPageSize.

Since this code will not function well with alignment > pagesize I'll add in
asserts to make sure no callers sneak in so we don't have to go through this
bughunt process again sometime down the line.  Will that hurt the upstreaming
effort?

[Anton Muhin, 2009-09-24 22:12:09]

I am fine w/ it.

yours,
anton.

On Fri, Sep 25, 2009 at 1:56 AM,  <jamesr@chromium.org> wrote:
> On 2009/09/24 21:43:44, antonm wrote:
>>
>> Jim, thanks a lot for comments!
>
>> http://codereview.chromium.org/235003/diff/1/2
>> File third_party/tcmalloc/port.cc (right):
>
>> http://codereview.chromium.org/235003/diff/1/2#newcode209
>> Line 209: void* result =3D VirtualAlloc(0, size + extra,
>> On 2009/09/24 21:17:18, jar wrote:
>> > Note that the requested size potentially includes extra.
>> >
>> > As a result, I would expect that it would be better to inform the call=
er
>
> that
>>
>> > the actual_size is the total requested size, including extra.
>> >
>> > What am I missing?
>
>> I put a TODO instead of fixing alignment stuff for one reason: it's kind
>> of
>> orthogonal to the problem I try to solve :)
>
>> And it is apparently not at all easy. =A0For example, it's not only a ma=
tter
>> of
>> adding extra, somehow I need to communicate back original pointer, befor=
e
>
> adding
>>
>> adjust, otherwise coalescing with the previous span would suffer, correc=
t?
>
> And
>>
>> release could should be adjusted as well.
>
>> Luckily, I would hope tcmalloc is never or rarely invoked with alignment=
 >
>> pagesize. =A0And if this change goes in, it'd require > 64K alignment to
>> expose
>> the bug.
>
>> So I thought to postpone fixing those issues. =A0Was it a reasonable cho=
ice?
>
> There are no callers currently with odd alignment requirements - in the
> Chromium
> codebase today all callers either use the default argument alignment=3D0
> (which is
> floored up to pagesize) or they use kPageSize.
>
> Since this code will not function well with alignment > pagesize I'll add=
 in
> asserts to make sure no callers sneak in so we don't have to go through t=
his
> bughunt process again sometime down the line. =A0Will that hurt the
> upstreaming
> effort?
>
>
>
> http://codereview.chromium.org/235003
>

[jamesr, 2009-09-24 22:33:23]

New patch in http://codereview.chromium.org/222028

[jamesr, 2009-09-25 01:42:46]

On 2009/09/24 22:33:23, jamesr wrote:
> New patch in http://codereview.chromium.org/222028

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

[Anton Muhin, 2009-09-25 12:41:42]

Thinking a bit more about it, is TCMalloc_SystemAlloc a right place
for alignment at all?  I'd rather remove alignment treatment at all,
does anybody know about any compelling reasons to support alignment in
TCMalloc_SystemAlloc at all?

[should I have moved it to some other list?]

yours,
anton.

On Fri, Sep 25, 2009 at 1:43 AM,  <antonm@chromium.org> wrote:
> Jim, thanks a lot for comments!
>
>
> http://codereview.chromium.org/235003/diff/1/2
> File third_party/tcmalloc/port.cc (right):
>
> http://codereview.chromium.org/235003/diff/1/2#newcode209
> Line 209: void* result =3D VirtualAlloc(0, size + extra,
> On 2009/09/24 21:17:18, jar wrote:
>>
>> Note that the requested size potentially includes extra.
>
>> As a result, I would expect that it would be better to inform the
>
> caller that
>>
>> the actual_size is the total requested size, including extra.
>
>> What am I missing?
>
> I put a TODO instead of fixing alignment stuff for one reason: it's kind
> of orthogonal to the problem I try to solve :)
>
> And it is apparently not at all easy. =A0For example, it's not only a
> matter of adding extra, somehow I need to communicate back original
> pointer, before adding adjust, otherwise coalescing with the previous
> span would suffer, correct? =A0And release could should be adjusted as
> well.
>
> Luckily, I would hope tcmalloc is never or rarely invoked with alignment
>>
>> pagesize. =A0And if this change goes in, it'd require > 64K alignment to
>
> expose the bug.
>
> So I thought to postpone fixing those issues. =A0Was it a reasonable
> choice?
>
> http://codereview.chromium.org/235003
>