Revert further back to MBelshe's baseline forking TCMalloc...

Revert further back to MBelshe's baseline forking TCMalloc

This changes to decommitting in all paths through the
page_heap delete method (which adds spans to the free lists).

r=mbelshe,jamesr

[jar (doing other things), 2009-10-02 23:27:46]

James: If you have a chance to comment, that would be nice.

I'm still debugging an issue in my tree, but I *think* this may be good code
(and my tree has some separable horkage).

Mike: Why don't you hold off on the review.

Antonm: just FYI.

[jar (doing other things), 2009-10-03 01:59:29]

Based on discussion with JamesR (thanks!), this version better mimics Mike's
original code, and is much simpler.

[jamesr, 2009-10-05 17:49:04]

LGTM!

[Mike Belshe, 2009-10-05 17:52:57]

lgtm

[jar (doing other things), 2009-10-05 18:51:14]

I committed the CL, but gcl did not close the issue, so I'll do so manually. 
The message was:

Committed revision 28006.
Loaded authentication cookies from c:\Users\jar\.codereview_upload_cookies
Error accessing url /255067/close

[Anton Muhin, 2009-10-06 10:26:57]

Jim, all,

are those (quoted from review of
http://codereview.chromium.org/242088) applicable?

quote {
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).

} // quote

yours,
anton.

On Mon, Oct 5, 2009 at 10:51 PM,  <jar@chromium.org> wrote:
> I committed the CL, but gcl did not close the issue, so I'll do so manually.
> The message was:
>
> Committed revision 28006.
> Loaded authentication cookies from c:\Users\jar\.codereview_upload_cookies
> Error accessing url /255067/close
>
> http://codereview.chromium.org/255067
>

[jar (doing other things), 2009-10-06 16:08:51]

On Tue, Oct 6, 2009 at 3:26 AM, Anton Muhin <antonm@google.com> wrote:

> Jim, all,
>
> are those (quoted from review of
> http://codereview.chromium.org/242088) applicable?
>
> quote {
> 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.
>

Committed spans will surely come back to existance as our policy changes...
so I think it would be a bad thing to rip out the code in the interim.

You are correct that since coalescing always results in a decommitted
region, all free spans end up in the returned heap.  As I pointed out in a
comment (see the "TBD(jar"), this even applies to initial allocations via
GrowHeap(), which means we reserve/commit, then decomit (as we coalesce) and
then commit (as we carve).  This is suboptimal, but we need benchmarks of
the system call to decide how bad it is.


>
> 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).
>

That is one way to go.  The interesting question in such an optimization is
how often we coalesce?  As hinted above, it may well be that when we pass
through the coalescing, if we don't do any coalescing, we *might* want to
leave the state alone, especially during a grow heap.  All this is TBD.


>
> 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).
>

Remaining closer to the open source project seems like a good thing, so I'd
rather not diverge so much.  As I said, I suspect we'll need this code
shortly anyway.

Jim



>
> } // quote
>
> yours,
> anton.
>
> On Mon, Oct 5, 2009 at 10:51 PM,  <jar@chromium.org> wrote:
> > I committed the CL, but gcl did not close the issue, so I'll do so
> manually.
> > The message was:
> >
> > Committed revision 28006.
> > Loaded authentication cookies from
> c:\Users\jar\.codereview_upload_cookies
> > Error accessing url /255067/close
> >
> > http://codereview.chromium.org/255067
> >
>