Clean up and clarify PartitionAlloc

Clean up and clarify PartitionAlloc

A partition page is not ‘typically’ 16 kB, it is always defined to be 16 kB.
Also drive-by fixes for line wrapping and typos.

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

[Ruud van Asseldonk, 2015-09-02 15:31:56]

ruuda@google.com changed reviewers:
+ haraken@chromium.org, primiano@chromium.org

[Ruud van Asseldonk, 2015-09-02 15:31:57]

In my quest to understand partitionAlloc, I encountered a very misleading
comment, and some typos/line wrapping issues which are fixed by this CL.
Furthermore, this tries to unify the two functions for rounding sizes, and make
the distinction between rounding and aligning.

[Primiano Tucci (use gerrit), 2015-09-02 21:58:20]

Hmm ok for fixing comments and improving names (not sure if that justifies a
standalone CL though) but I have some big doubts on the casts below. Let's
discuss this offline tomorrow.

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

https://codereview.chromium.org/1323093004/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:514: size_t addr = reinterpret_cast<size_t>(ptr);
Hmm not sure I follow this (And below). What's wrong with size_t? why (ab)using
char*?

[haraken, 2015-09-02 23:29:35]

On 2015/09/02 21:58:20, Primiano Tucci wrote:
> Hmm ok for fixing comments and improving names (not sure if that justifies a
> standalone CL though) but I have some big doubts on the casts below. Let's
> discuss this offline tomorrow.
> 
>
https://codereview.chromium.org/1323093004/diff/1/Source/wtf/PartitionAlloc.cpp
> File Source/wtf/PartitionAlloc.cpp (right):
> 
>
https://codereview.chromium.org/1323093004/diff/1/Source/wtf/PartitionAlloc.c...
> Source/wtf/PartitionAlloc.cpp:514: size_t addr =
reinterpret_cast<size_t>(ptr);
> Hmm not sure I follow this (And below). What's wrong with size_t? why
(ab)using
> char*?

As Primiano points out, shall we land a comment-only & naming-only change first?

[Ruud van Asseldonk, 2015-09-03 08:32:25]

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

https://codereview.chromium.org/1323093004/diff/1/Source/wtf/PartitionAlloc.c...
Source/wtf/PartitionAlloc.cpp:514: size_t addr = reinterpret_cast<size_t>(ptr);
On 2015/09/02 at 21:58:19, Primiano Tucci wrote:
> Hmm not sure I follow this (And below). What's wrong with size_t? why
(ab)using char*?

This simply moves the casts that were there already into the function. (See
lines 543, 1146, 1149, 1193, 1194.) Previously the function to round sizes was
used to align pointers, by reinterpreting the pointer as an integer, which
happens to be possible on the platforms we build for.

[Primiano Tucci (use gerrit), 2015-09-03 10:31:13]

LGTM (haraken to stamp) thanks for fixing the comments.
Let's move the discussion about the functions / rounding to the another CL.

[haraken, 2015-09-03 10:46:56]

LGTM

[Ruud van Asseldonk, 2015-09-03 11:07:30]

The CQ bit was checked by ruuda@google.com

[commit-bot: I haz the power, 2015-09-03 11:07:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323093004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323093004/20001

[commit-bot: I haz the power, 2015-09-03 11:48:47]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201720

[Jens Widell, 2015-09-03 13:11:45]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2015-09-03 13:11:46]

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

https://codereview.chromium.org/1323093004/diff/20001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.h:144: // What we're actually doing is avoiding
filling the full partition page (16 KB)
I guess this comment meant to discuss the "bucket pages" (my own term, not used
in the code) that are multiples of partition pages, and indeed "typically 16 KB"
(i.e. a single partition page) at least for smaller bucket sizes. "Bucket pages"
are logically what PartitionBucket::activePagesHead point to.

In other words, |X x partition page| makes up the storage for a "bucket page" of
which |Y x bucket slot size| will be used, thus leaving |Z x system page size|
memory permanently unused. (Z might be zero.)

And the restricted freelist filling discussed here avoids filling up the whole
bucket page, by touching as few system pages as possible. It doesn't care
specifically about partition page boundaries.