PartitionAlloc: make PartitionPurgeDiscardUnusedSystemPages discard more.

PartitionAlloc: make PartitionPurgeDiscardUnusedSystemPages discard more.

I think this is the final source of easily discardable pages: truncate the
number of slots in a slot span, if the trailing slot(s) are unused.

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

[Chris Evans, 2015-06-24 00:50:36]

cevans@chromium.org changed reviewers:
+ haraken@chromium.org

[Chris Evans, 2015-06-24 00:51:45]

This is getting a bit complicated, but I think it's worth it because we've now
exhausted all the different tricks and techniques to find discardable pages.

I suggest we land this patch, then think about the risks of calling this new
API, landing any follow-up safety defenses as separate CLs.

[haraken, 2015-06-24 01:10:16]

Nice idea!

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

https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:1092: memset(slotUsage, 1, sizeof(slotUsage));

  memset(slotUsage, 1, sizeof(numSlots));

might be better (although it won't affect correctness).

https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:1126: endPtr = beginPtr + slotSize;

Shouldn't this be:

  endPtr = beginPtr + slotSize * truncatedSlots

?

https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:1128: endPtr =
reinterpret_cast<char*>(partitionRoundUpToSystemPage(reinterpret_cast<size_t>(endPtr)));

Maybe worth having a comment why this is partitionRound*Up*ToSystemPage, not
partitionRound*Down*ToSystemPage.

Also shall we add an ASSERT to verify that endptr is within the page's slot
span?

https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:1136: page->numUnprovisionedSlots +=
truncatedSlots;

Shall we add:

  ASSERT(page->numUnprovisionedSlots + page->numAllocatedSlots ==
bucketNumSlots);

?

https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:1141: while (entry) {

A more straightforward way would be just to scan all the slots in the page and
rebuild the freelist.

for (size_t slotIndex = 0; slotIndex < numSlots; slotIndex++) {
  if (!slotUsage[slotIndex]) {
    // add this slot to the freelist.
  }
}

[Chris Evans, 2015-06-24 05:58:45]

On 2015/06/24 01:10:16, haraken wrote:
> Nice idea!
> 
>
https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.cpp
> File Source/wtf/PartitionAlloc.cpp (right):
> 
>
https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
> Source/wtf/PartitionAlloc.cpp:1092: memset(slotUsage, 1, sizeof(slotUsage));
> 
>   memset(slotUsage, 1, sizeof(numSlots));
> 
> might be better (although it won't affect correctness).

Done (I think you meant numSlots, not sizeof(numSlots))

> 
>
https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
> Source/wtf/PartitionAlloc.cpp:1126: endPtr = beginPtr + slotSize;
> 
> Shouldn't this be:
> 
>   endPtr = beginPtr + slotSize * truncatedSlots

Oh! Yes indeed, thanks. Test added to catch this too.

> 
> ?
> 
>
https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
> Source/wtf/PartitionAlloc.cpp:1128: endPtr =
>
reinterpret_cast<char*>(partitionRoundUpToSystemPage(reinterpret_cast<size_t>(endPtr)));
> 
> Maybe worth having a comment why this is partitionRound*Up*ToSystemPage, not
> partitionRound*Down*ToSystemPage.

Done.

> 
> Also shall we add an ASSERT to verify that endptr is within the page's slot
> span?

Done.

> 
>
https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
> Source/wtf/PartitionAlloc.cpp:1136: page->numUnprovisionedSlots +=
> truncatedSlots;
> 
> Shall we add:
> 
>   ASSERT(page->numUnprovisionedSlots + page->numAllocatedSlots ==
> bucketNumSlots);

I don't think that ASSERT holds. If the first slot is free and the second one in
use, we'd get (2 + 1 == 4). There's another nearby ASSERT that checks that the
number of freelist pointers written adds up.

> 
> ?
> 
>
https://codereview.chromium.org/1203893002/diff/1/Source/wtf/PartitionAlloc.c...
> Source/wtf/PartitionAlloc.cpp:1141: while (entry) {
> 
> A more straightforward way would be just to scan all the slots in the page and
> rebuild the freelist.
> 
> for (size_t slotIndex = 0; slotIndex < numSlots; slotIndex++) {
>   if (!slotUsage[slotIndex]) {
>     // add this slot to the freelist.
>   }
> }

Ah, thanks. I was struggling with that. Done.

[haraken, 2015-06-24 13:53:07]

LGTM

https://codereview.chromium.org/1203893002/diff/20001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/1203893002/diff/20001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:1156: if (truncatedSlots && discard) {

if (truncatedSlots && discard) => if (unprovisionedBytes > 0 && discard)

And we can add ASSERT(truncatedSlots > 0).

[Chris Evans, 2015-06-24 18:32:07]

On 2015/06/24 13:53:07, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/1203893002/diff/20001/Source/wtf/PartitionAll...
> File Source/wtf/PartitionAlloc.cpp (right):
> 
>
https://codereview.chromium.org/1203893002/diff/20001/Source/wtf/PartitionAll...
> Source/wtf/PartitionAlloc.cpp:1156: if (truncatedSlots && discard) {
> 
> if (truncatedSlots && discard) => if (unprovisionedBytes > 0 && discard)
> 
> And we can add ASSERT(truncatedSlots > 0).

Done.

[Chris Evans, 2015-06-24 18:32:11]

The CQ bit was checked by cevans@chromium.org

[Chris Evans, 2015-06-24 18:32:11]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1203893002/#ps40001
(title: "Action review comments.")

[commit-bot: I haz the power, 2015-06-24 18:33:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203893002/40001

[commit-bot: I haz the power, 2015-06-24 18:58:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 18:58:01]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/68173)

[Chris Evans, 2015-06-24 19:22:43]

The CQ bit was checked by cevans@chromium.org

[Chris Evans, 2015-06-24 19:22:44]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1203893002/#ps60001
(title: "Compile fix.")

[commit-bot: I haz the power, 2015-06-24 19:22:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203893002/60001

[commit-bot: I haz the power, 2015-06-24 20:32:31]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197776