PartitionAlloc: wait just a little while before actually freeing empty pages.

PartitionAlloc: wait just a little while before actually freeing empty pages.

We hold a small ring-list cache of recently-emptied pages, and only free the
oldest entry in the ring list when the cache is full. This makes us hold on to
a little bit of unused memory, but in return we do now eventually free the last
slot span in any given bucket, which would have been held indefinitely before.

This patch is a sufficient generic replacement for the bandaid added in
https://code.google.com/p/chromium/issues/detail?id=330798; accordingly, the
bandaid is removed. In fact, we now seem to be +4% faster than _before_ the
original regression.

BUG=332282
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165112

[Chris Evans, 2014-01-14 22:05:23]

Ok, I think this is the simple key to getting the remainder landed again.

[Tom Sepez, 2014-01-14 22:17:10]

There's probably a simpler LRU scheme where you only need one index:
1. if there's a page at the current index, free it for realz, 2. store the new
page there and preserve the index in the page, 
3. bump the index in a circular manner.

If a page gets yanked, just set its index to NULL, when we come back around we
may skip to #2.

You don't really need to count how many we got outstanding, just that if its in
there long enough so that we wrap, its fine to toss it.

I think thats less error prone.

https://codereview.chromium.org/133863006/diff/1/Source/wtf/PartitionAlloc.cpp
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/133863006/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:628: tailIndex -= kMaxFreeableSpans;
nit: assert that it's in range after subtracting.

[Chris Evans, 2014-01-14 22:57:39]

On 2014/01/14 22:17:10, Tom Sepez wrote:
> There's probably a simpler LRU scheme where you only need one index:
> 1. if there's a page at the current index, free it for realz, 2. store the new
> page there and preserve the index in the page, 
> 3. bump the index in a circular manner.
> 
> If a page gets yanked, just set its index to NULL, when we come back around we
> may skip to #2.
> 
> You don't really need to count how many we got outstanding, just that if its
in
> there long enough so that we wrap, its fine to toss it.
> 
> I think thats less error prone.
> 
> https://codereview.chromium.org/133863006/diff/1/Source/wtf/PartitionAlloc.cpp
> File Source/wtf/PartitionAlloc.cpp (right):
> 
>
https://codereview.chromium.org/133863006/diff/1/Source/wtf/PartitionAlloc.cp...
> Source/wtf/PartitionAlloc.cpp:628: tailIndex -= kMaxFreeableSpans;
> nit: assert that it's in range after subtracting.

I think I get / did what you meant? PTAL.

[Tom Sepez, 2014-01-14 23:08:46]

https://codereview.chromium.org/133863006/diff/70001/Source/wtf/PartitionAllo...
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/133863006/diff/70001/Source/wtf/PartitionAllo...
Source/wtf/PartitionAlloc.cpp:606: ASSERT(!pageToFree || pageToFree ==
root->globalEmptyPageRing[pageToFree->freeCacheIndex - 1]);
nit: mabye this assert moves inside if (pageToFree) to avoid double-testing the
!pageToFree in the assert. Maybe both asserts.

https://codereview.chromium.org/133863006/diff/70001/Source/wtf/PartitionAllo...
Source/wtf/PartitionAlloc.cpp:620: page->freeCacheIndex = currentIndex + 1;
why +1 ? Maybe 0xffff as magic value not in slot? or maybe uint8_t + bool to
tell if in table or onot.

https://codereview.chromium.org/133863006/diff/70001/Source/wtf/PartitionAllo...
Source/wtf/PartitionAlloc.cpp:621: ++currentIndex;
nit: do currentIndex++ first, then assign to avoid the +1.

[Chris Evans, 2014-01-14 23:15:10]

On 2014/01/14 23:08:46, Tom Sepez wrote:
>
https://codereview.chromium.org/133863006/diff/70001/Source/wtf/PartitionAllo...
> File Source/wtf/PartitionAlloc.cpp (right):
> 
>
https://codereview.chromium.org/133863006/diff/70001/Source/wtf/PartitionAllo...
> Source/wtf/PartitionAlloc.cpp:606: ASSERT(!pageToFree || pageToFree ==
> root->globalEmptyPageRing[pageToFree->freeCacheIndex - 1]);
> nit: mabye this assert moves inside if (pageToFree) to avoid double-testing
the
> !pageToFree in the assert. Maybe both asserts.

Ah, yes. Done.

> 
>
https://codereview.chromium.org/133863006/diff/70001/Source/wtf/PartitionAllo...
> Source/wtf/PartitionAlloc.cpp:620: page->freeCacheIndex = currentIndex + 1;
> why +1 ? Maybe 0xffff as magic value not in slot? or maybe uint8_t + bool to
> tell if in table or onot.

I couldn't quite convince myself that either option is really any simpler? I
added a comment, if that helps?
I also added another ASSERT checking we have a cache index where we expect one.

> 
>
https://codereview.chromium.org/133863006/diff/70001/Source/wtf/PartitionAllo...
> Source/wtf/PartitionAlloc.cpp:621: ++currentIndex;
> nit: do currentIndex++ first, then assign to avoid the +1.

Done.

[Tom Sepez, 2014-01-14 23:25:49]

lgtm

[commit-bot: I haz the power, 2014-01-14 23:40:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cevans@chromium.org/133863006/200001

[commit-bot: I haz the power, 2014-01-15 09:46:10]

Change committed as 165112