For large (but not direct mapped) allocations, track the real allocation size.

For large (but not direct mapped) allocations, track the real allocation size.

Allocations above 64kB can be tracked more accurately because we have spare
metadata space where we can store the exact allocation size. For now, we use
this to report more accurate statistics. But we will probably wire it up to
partitionPurgeMemory in a future CL.

During work on this CL, a function to round up to system page size was
factored out, and the control flow of partitionAllocSlowPath was tided up to
fall down to the return's at the tail of the function in more cases.

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

[Chris Evans, 2015-06-05 21:40:41]

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

[haraken, 2015-06-06 15:07:54]

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

https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:809: if (UNLIKELY(rawSizePtr != nullptr))

Given that this is a alow path, we might want to drop the UNLIKELY (unless it
really matters for performance).

https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:812: if (LIKELY(ret != nullptr))

This looks a bit weird. Maybe we can create a helper function like
partitionPageSetRawSize() and call it at line 786 and line 808. Then we can just
return immediately after line 786.

https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:1074: statsOut->activeBytes +=
static_cast<uint32_t>(partitionRoundUpToSystemPage(*rawSizePtr));

Help me understand: Why is this more accurate than "page->numAllocatedSlots *
statsOut->bucketSlotSize"? For activeBytes, we want to report the size that is
actually used by objects. Then shouldn't we just report "page->numAllocatedSlots
* statsOut->bucketSlotSize" for >64 KB slots as well?

[Chris Evans, 2015-06-10 00:44:06]

On 2015/06/06 15:07:54, haraken wrote:
>
https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.cpp
> File Source/wtf/PartitionAlloc.cpp (right):
> 
>
https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.c...
> Source/wtf/PartitionAlloc.cpp:809: if (UNLIKELY(rawSizePtr != nullptr))
> 
> Given that this is a alow path, we might want to drop the UNLIKELY (unless it
> really matters for performance).

I don't think it realistically matters for performance, but I like it for the
documentation aspect, and it also matches the general style of the file to
include these macros a lot.

> 
>
https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.c...
> Source/wtf/PartitionAlloc.cpp:812: if (LIKELY(ret != nullptr))
> 
> This looks a bit weird. Maybe we can create a helper function like
> partitionPageSetRawSize() and call it at line 786 and line 808. Then we can
just
> return immediately after line 786.

Agreed; done.

> 
>
https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.c...
> Source/wtf/PartitionAlloc.cpp:1074: statsOut->activeBytes +=
> static_cast<uint32_t>(partitionRoundUpToSystemPage(*rawSizePtr));
> 
> Help me understand: Why is this more accurate than "page->numAllocatedSlots *
> statsOut->bucketSlotSize"? For activeBytes, we want to report the size that is
> actually used by objects. Then shouldn't we just report
"page->numAllocatedSlots
> * statsOut->bucketSlotSize" for >64 KB slots as well?

Let's consider the first "interesting" bucket, which is the one that stores
allocations from 65537 - 73728 bytes. For this bucket, numAllocated slots will
always be 0 or 1, and bucketSlotSize is 73728.
It gets interesting if we have an allocation in the slot which is between 65537
- 69632 bytes. In this case, there's potentially a freeable 4096-byte system
page at the end of the allocation slot.
So that's the difference here: actual slot size vs. the object put in it. For
these very big buckets, the difference is interesting because it might represent
something freeable. I'm putting in the infrastructure needed to make
comprehensive freeing decisions, in the hope of squeezing out a few more KB here
and there for the code purple :)

[haraken, 2015-06-10 00:53:54]

On 2015/06/10 00:44:06, Chris Evans wrote:
> On 2015/06/06 15:07:54, haraken wrote:
> >
>
https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.cpp
> > File Source/wtf/PartitionAlloc.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.c...
> > Source/wtf/PartitionAlloc.cpp:809: if (UNLIKELY(rawSizePtr != nullptr))
> > 
> > Given that this is a alow path, we might want to drop the UNLIKELY (unless
it
> > really matters for performance).
> 
> I don't think it realistically matters for performance, but I like it for the
> documentation aspect, and it also matches the general style of the file to
> include these macros a lot.

OK. Blink normally prefers not using LIKELY/UNLIKELY in places where performance
doesn't matter.

>
https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.c...
> > Source/wtf/PartitionAlloc.cpp:812: if (LIKELY(ret != nullptr))
> > 
> > This looks a bit weird. Maybe we can create a helper function like
> > partitionPageSetRawSize() and call it at line 786 and line 808. Then we can
> just
> > return immediately after line 786.
> 
> Agreed; done.
> 
> > 
> >
>
https://codereview.chromium.org/1153473011/diff/1/Source/wtf/PartitionAlloc.c...
> > Source/wtf/PartitionAlloc.cpp:1074: statsOut->activeBytes +=
> > static_cast<uint32_t>(partitionRoundUpToSystemPage(*rawSizePtr));
> > 
> > Help me understand: Why is this more accurate than "page->numAllocatedSlots
*
> > statsOut->bucketSlotSize"? For activeBytes, we want to report the size that
is
> > actually used by objects. Then shouldn't we just report
> "page->numAllocatedSlots
> > * statsOut->bucketSlotSize" for >64 KB slots as well?
> 
> Let's consider the first "interesting" bucket, which is the one that stores
> allocations from 65537 - 73728 bytes. For this bucket, numAllocated slots will
> always be 0 or 1, and bucketSlotSize is 73728.
> It gets interesting if we have an allocation in the slot which is between
65537
> - 69632 bytes. In this case, there's potentially a freeable 4096-byte system
> page at the end of the allocation slot.
> So that's the difference here: actual slot size vs. the object put in it. For
> these very big buckets, the difference is interesting because it might
represent
> something freeable. I'm putting in the infrastructure needed to make
> comprehensive freeing decisions, in the hope of squeezing out a few more KB
here
> and there for the code purple :)

Thanks for the clarification! Makes sense.

[haraken, 2015-06-10 00:54:01]

LGTM

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

https://codereview.chromium.org/1153473011/diff/20001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:1073: size_t* rawSizePtr =
partitionPageGetRawSizePtr(const_cast<PartitionPage*>(page));

Nit: It looks a bit tidier to introduce partitionPageGetRawSize(page) (which
returns 0 if the raw size is not available).

[Chris Evans, 2015-06-10 18:49:42]

On 2015/06/10 00:54:01, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/1153473011/diff/20001/Source/wtf/PartitionAll...
> File Source/wtf/PartitionAlloc.cpp (right):
> 
>
https://codereview.chromium.org/1153473011/diff/20001/Source/wtf/PartitionAll...
> Source/wtf/PartitionAlloc.cpp:1073: size_t* rawSizePtr =
> partitionPageGetRawSizePtr(const_cast<PartitionPage*>(page));
> 
> Nit: It looks a bit tidier to introduce partitionPageGetRawSize(page) (which
> returns 0 if the raw size is not available).

Agreed, done.

[Chris Evans, 2015-06-10 18:50:04]

The CQ bit was checked by cevans@chromium.org

[Chris Evans, 2015-06-10 18:50:04]

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/1153473011/#ps40001
(title: "More review feedback.")

[commit-bot: I haz the power, 2015-06-10 18:50:17]

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

[commit-bot: I haz the power, 2015-06-10 19:49:11]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196893