Decommits when coallescing spans instead of committing.

Decommits when coallescing spans instead of committing.  This results in memory being decommitted very aggressively which can greatly decrease the amount of committed memory in a renderer or the browser process.  Based on local tests on windows it causes a perf regression of <2% on the V8 and Dromaeo DOM Core benchmarks and no impact on SunSpider.  It decreased the memory footprint of a renderer after running Dromaeo's DOM Core suite from 330MB+ down to <100MB.

Mike Belshe has landed a similar patch in the past, but the behavior was later reverted due to perf concerns.  I think this patch is much too aggressive in decommitting memory but given the somewhat out of control memory use of chromium in some cases currently I'd suggest it's better than what we have right now.  Over time the decommitting strategy should be tuned to optimize both perf and memory size.  Right now we seem to be blowing a lot of memory for not much of a win.

[jamesr, 2009-10-01 00:23:46]

Thoughts on trying this?

[Mike Belshe, 2009-10-01 02:39:48]

I like the algorithm - it's basically the same as the revision before Anton's
change.

The trunk code does Commit on coalesce; I changed it to Decommit on coalesce;
Anton changed it back to Commit on coalesce with background purging.

Your algorithm has optimistic Decommit, like mine, and leaves the background
purging from Anton's fix in place.

http://codereview.chromium.org/242088/diff/6001/6002
File third_party/tcmalloc/page_heap.cc (right):

http://codereview.chromium.org/242088/diff/6001/6002#newcode254
Line 254: DLL_Prepend(&free_[span->length].normal, span);
I think this is a bug -

DLL_Prepend(&free_[span->length].returned, span);

?

http://codereview.chromium.org/242088/diff/6001/6002#newcode256
Line 256: DLL_Prepend(&large_.normal, span);
ditto

[Mike Belshe, 2009-10-01 02:41:59]

Given that we have the background purger now, could we tweak this file to do:

if (coalescing into bigger span)
  use the returned|normal list from the bigger span
else
  decommit the coalesced span

?

I'm not sure how to measure which is better right now.  I'd be in favor of
landing this fix now, with more experimentation to follow.

[antonm, 2009-10-01 11:59:27]

Several more things to consider:

1) I am almost sure (but please, double check) that this change makes normal
spans obsolete: before this change both normal and returned spans exist, with
this change span is always returned back to the system.  So I'd suggest to make
it explicit in the code, ideally nuking normal and span location altogether. 
That forks us further from tcmalloc though.

2) if picked this route, you shouldn't commit the range in
TCMalloc_SystemAlloc---it would get uncommitted immediately.  That would be a
nice step into right direction (remember that ideally we'd like to virtualalloc
one huge uncommitted block and chop it later).

3) If my analysis in regard of normal/returned lists is correct, you should
probably nuke MallocExtensions->ReleaseFreePages() from
RenderThread::IdleNotifiction (note it should remove an include and thus alter
file listing module deps).

It's up to you if you'd like to address the issues above (if at all) in one big
cl or several smaller ones.  The only thing I'd ask for---if possible, let's
isolate all performance affecting changes into one CL---it should make it easier
to see perf impact.

And if possible, I'd like to have a comparison of two approaches: one which
decommits and another which uses periodic ReleaseFreePages.  I believe that
second both is more perfomant and eventually would uncommit all the pages.

What do you think?

BTW, regarding SunSpider and V8---I won't be surprised (but don't know for sure)
if those mostly allocate in V8 and thus do not use tcmalloc (or use it minimally
to allocate global handles)

http://codereview.chromium.org/242088/diff/6001/6002
File third_party/tcmalloc/page_heap.cc (right):

http://codereview.chromium.org/242088/diff/6001/6002#newcode251
Line 251: Event(span, 'D', span->length);
why you don't decommit the span itself?  is it by intent or just an oversight?

BTW, I won't be surprise that if you change that, it could have performance
implications (if committing is cheaper for already committed pages)

http://codereview.chromium.org/242088/diff/6001/6002#newcode254
Line 254: DLL_Prepend(&free_[span->length].normal, span);
On 2009/10/01 02:39:48, Mike Belshe wrote:
> I think this is a bug -
> 
> DLL_Prepend(&free_[span->length].returned, span);
> 
> ?

That's definitely a bug and I would expect immediate crash under real load---you
put into normal list spans in mixed state (some pages are committed and some are
not).

[jamesr, 2009-10-02 00:52:27]

Given that jemalloc is the default for the next week I think I'll leave this
patch on hold and try to get some more formal metrics in place to keep tabs on
the effects of changes like this.   Thanks for looking at it.

On 2009/10/01 11:59:27, antonm wrote:
> Several more things to consider:
> 
> 1) I am almost sure (but please, double check) that this change makes normal
> spans obsolete: before this change both normal and returned spans exist, with
> this change span is always returned back to the system.  So I'd suggest to
make
> it explicit in the code, ideally nuking normal and span location altogether. 
> That forks us further from tcmalloc though.
> 
> 2) if picked this route, you shouldn't commit the range in
> TCMalloc_SystemAlloc---it would get uncommitted immediately.  That would be a
> nice step into right direction (remember that ideally we'd like to
virtualalloc
> one huge uncommitted block and chop it later).
> 
> 3) If my analysis in regard of normal/returned lists is correct, you should
> probably nuke MallocExtensions->ReleaseFreePages() from
> RenderThread::IdleNotifiction (note it should remove an include and thus alter
> file listing module deps).
> 
> It's up to you if you'd like to address the issues above (if at all) in one
big
> cl or several smaller ones.  The only thing I'd ask for---if possible, let's
> isolate all performance affecting changes into one CL---it should make it
easier
> to see perf impact.
> 
> And if possible, I'd like to have a comparison of two approaches: one which
> decommits and another which uses periodic ReleaseFreePages.  I believe that
> second both is more perfomant and eventually would uncommit all the pages.
> 
> What do you think?
> 
> BTW, regarding SunSpider and V8---I won't be surprised (but don't know for
sure)
> if those mostly allocate in V8 and thus do not use tcmalloc (or use it
minimally
> to allocate global handles)

Right, so it's a bit surprising that V8 regressed at all.


> 
> http://codereview.chromium.org/242088/diff/6001/6002
> File third_party/tcmalloc/page_heap.cc (right):
> 
> http://codereview.chromium.org/242088/diff/6001/6002#newcode251
> Line 251: Event(span, 'D', span->length);
> why you don't decommit the span itself?  is it by intent or just an oversight?
> 
> BTW, I won't be surprise that if you change that, it could have performance
> implications (if committing is cheaper for already committed pages)
> 
> http://codereview.chromium.org/242088/diff/6001/6002#newcode254
> Line 254: DLL_Prepend(&free_[span->length].normal, span);
> On 2009/10/01 02:39:48, Mike Belshe wrote:
> > I think this is a bug -
> > 
> > DLL_Prepend(&free_[span->length].returned, span);
> > 
> > ?
> 
> That's definitely a bug and I would expect immediate crash under real
load---you
> put into normal list spans in mixed state (some pages are committed and some
are
> not).

Yeah, putting it in the wrong freelist is definitely a bug.

[Mads Ager (chromium), 2009-10-02 06:59:37]

On Fri, Oct 2, 2009 at 2:52 AM,  <jamesr@chromium.org> wrote:
>
> Given that jemalloc is the default for the next week I think I'll leave t=
his
> patch on hold and try to get some more formal metrics in place to keep ta=
bs
> on
> the effects of changes like this. =A0 Thanks for looking at it.
>
> On 2009/10/01 11:59:27, antonm wrote:
>>
>> Several more things to consider:
>
>> 1) I am almost sure (but please, double check) that this change makes
>> normal
>> spans obsolete: before this change both normal and returned spans exist,
>> with
>> this change span is always returned back to the system. =A0So I'd sugges=
t to
>
> make
>>
>> it explicit in the code, ideally nuking normal and span location
>> altogether.
>> That forks us further from tcmalloc though.
>
>> 2) if picked this route, you shouldn't commit the range in
>> TCMalloc_SystemAlloc---it would get uncommitted immediately. =A0That wou=
ld
>> be a
>> nice step into right direction (remember that ideally we'd like to
>
> virtualalloc
>>
>> one huge uncommitted block and chop it later).
>
>> 3) If my analysis in regard of normal/returned lists is correct, you
>> should
>> probably nuke MallocExtensions->ReleaseFreePages() from
>> RenderThread::IdleNotifiction (note it should remove an include and thus
>> alter
>> file listing module deps).
>
>> It's up to you if you'd like to address the issues above (if at all) in
>> one
>
> big
>>
>> cl or several smaller ones. =A0The only thing I'd ask for---if possible,
>> let's
>> isolate all performance affecting changes into one CL---it should make i=
t
>
> easier
>>
>> to see perf impact.
>
>> And if possible, I'd like to have a comparison of two approaches: one
>> which
>> decommits and another which uses periodic ReleaseFreePages. =A0I believe
>> that
>> second both is more perfomant and eventually would uncommit all the page=
s.
>
>> What do you think?
>
>> BTW, regarding SunSpider and V8---I won't be surprised (but don't know f=
or
>
> sure)
>>
>> if those mostly allocate in V8 and thus do not use tcmalloc (or use it
>
> minimally
>>
>> to allocate global handles)
>
> Right, so it's a bit surprising that V8 regressed at all.

Pure JS perf did not seem to regress as far as I can tell on the perf
bots?  Only the benchmarks dealing with the DOM as far as I can tell
(page cyclers/DOM benchmarks)?

>> http://codereview.chromium.org/242088/diff/6001/6002
>> File third_party/tcmalloc/page_heap.cc (right):
>
>> http://codereview.chromium.org/242088/diff/6001/6002#newcode251
>> Line 251: Event(span, 'D', span->length);
>> why you don't decommit the span itself? =A0is it by intent or just an
>> oversight?
>
>> BTW, I won't be surprise that if you change that, it could have
>> performance
>> implications (if committing is cheaper for already committed pages)
>
>> http://codereview.chromium.org/242088/diff/6001/6002#newcode254
>> Line 254: DLL_Prepend(&free_[span->length].normal, span);
>> On 2009/10/01 02:39:48, Mike Belshe wrote:
>> > I think this is a bug -
>> >
>> > DLL_Prepend(&free_[span->length].returned, span);
>> >
>> > ?
>
>> That's definitely a bug and I would expect immediate crash under real
>
> load---you
>>
>> put into normal list spans in mixed state (some pages are committed and
>> some
>
> are
>>
>> not).
>
> Yeah, putting it in the wrong freelist is definitely a bug.
>
>
>
> http://codereview.chromium.org/242088
>

[Anton Muhin, 2009-10-02 09:27:41]

My guess would be that pure JS benchmarks do not use (or use
minimally) malloc/free as V8 manages its heap at the OS level.

yours,
anton.

On Fri, Oct 2, 2009 at 10:59 AM, Mads Sig Ager <ager@chromium.org> wrote:
> On Fri, Oct 2, 2009 at 2:52 AM, =A0<jamesr@chromium.org> wrote:
>>
>> Given that jemalloc is the default for the next week I think I'll leave =
this
>> patch on hold and try to get some more formal metrics in place to keep t=
abs
>> on
>> the effects of changes like this. =A0 Thanks for looking at it.
>>
>> On 2009/10/01 11:59:27, antonm wrote:
>>>
>>> Several more things to consider:
>>
>>> 1) I am almost sure (but please, double check) that this change makes
>>> normal
>>> spans obsolete: before this change both normal and returned spans exist=
,
>>> with
>>> this change span is always returned back to the system. =A0So I'd sugge=
st to
>>
>> make
>>>
>>> it explicit in the code, ideally nuking normal and span location
>>> altogether.
>>> That forks us further from tcmalloc though.
>>
>>> 2) if picked this route, you shouldn't commit the range in
>>> TCMalloc_SystemAlloc---it would get uncommitted immediately. =A0That wo=
uld
>>> be a
>>> nice step into right direction (remember that ideally we'd like to
>>
>> virtualalloc
>>>
>>> one huge uncommitted block and chop it later).
>>
>>> 3) If my analysis in regard of normal/returned lists is correct, you
>>> should
>>> probably nuke MallocExtensions->ReleaseFreePages() from
>>> RenderThread::IdleNotifiction (note it should remove an include and thu=
s
>>> alter
>>> file listing module deps).
>>
>>> It's up to you if you'd like to address the issues above (if at all) in
>>> one
>>
>> big
>>>
>>> cl or several smaller ones. =A0The only thing I'd ask for---if possible=
,
>>> let's
>>> isolate all performance affecting changes into one CL---it should make =
it
>>
>> easier
>>>
>>> to see perf impact.
>>
>>> And if possible, I'd like to have a comparison of two approaches: one
>>> which
>>> decommits and another which uses periodic ReleaseFreePages. =A0I believ=
e
>>> that
>>> second both is more perfomant and eventually would uncommit all the pag=
es.
>>
>>> What do you think?
>>
>>> BTW, regarding SunSpider and V8---I won't be surprised (but don't know =
for
>>
>> sure)
>>>
>>> if those mostly allocate in V8 and thus do not use tcmalloc (or use it
>>
>> minimally
>>>
>>> to allocate global handles)
>>
>> Right, so it's a bit surprising that V8 regressed at all.
>
> Pure JS perf did not seem to regress as far as I can tell on the perf
> bots? =A0Only the benchmarks dealing with the DOM as far as I can tell
> (page cyclers/DOM benchmarks)?
>
>>> http://codereview.chromium.org/242088/diff/6001/6002
>>> File third_party/tcmalloc/page_heap.cc (right):
>>
>>> http://codereview.chromium.org/242088/diff/6001/6002#newcode251
>>> Line 251: Event(span, 'D', span->length);
>>> why you don't decommit the span itself? =A0is it by intent or just an
>>> oversight?
>>
>>> BTW, I won't be surprise that if you change that, it could have
>>> performance
>>> implications (if committing is cheaper for already committed pages)
>>
>>> http://codereview.chromium.org/242088/diff/6001/6002#newcode254
>>> Line 254: DLL_Prepend(&free_[span->length].normal, span);
>>> On 2009/10/01 02:39:48, Mike Belshe wrote:
>>> > I think this is a bug -
>>> >
>>> > DLL_Prepend(&free_[span->length].returned, span);
>>> >
>>> > ?
>>
>>> That's definitely a bug and I would expect immediate crash under real
>>
>> load---you
>>>
>>> put into normal list spans in mixed state (some pages are committed and
>>> some
>>
>> are
>>>
>>> not).
>>
>> Yeah, putting it in the wrong freelist is definitely a bug.
>>
>>
>>
>> http://codereview.chromium.org/242088
>>
>