PartitionAlloc: Count direct mapped pages as committed

PartitionAlloc: Count direct mapped pages as committed

Extend committed pages counting to direct mapped allocations as well. We
already did adjust the total committed pages size when reallocating a
direct mapped allocation to reflect the difference, but since we didn't do
any counting when initially mapping or when unmapping, this would just
result in accounting errors.

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

[Jens Widell, 2014-10-14 10:54:58]

jl@opera.com changed reviewers:
+ cevans@chromium.org, tsepez@chromium.org

[Jens Widell, 2014-10-14 10:54:58]

PTAL

Changed as discussed in issue 636213002.

This makes partitionFull() less likely to occur on 64-bit without actual
excessive memory usage. On 32-bit, we're probably about as likely to run out of
actual address space (and hit partitionOutOfMemory()) as we were to hit
partition full before, but things shouldn't be worse there at least.

This patch also includes direct mapped allocations in the measurement; they were
completely unaccounted for earlier (and did get the committed pages counter
slightly wrong if in-place reallocated.) An alternative approach here would of
course remove the slight misadjustment done when reallocating, and leave direct
mapped allocations unaccounted (if we think they should be.)

[Chris Evans, 2014-10-15 22:06:17]

Hmm. I'm actually leaning to just getting rid of the "partitionFull" concept. I
can't think of any critical reason why we have this logic. Tom, can you
remember? It's not even that effective as an anti-spray measure -- for example,
the accounting can be currently bypassed with "direct mapped" sections.

Unless Tom comes back with anything, I'll approve a CL to get rid of
partitionFull(). It's simpler.

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

https://codereview.chromium.org/649583004/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:581: root->totalSizeOfCommittedPages += size;
I think we might as well try and be fully accurate: the metadata page will also
be committed so hows about "size + kSystemPageSize" ?

https://codereview.chromium.org/649583004/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:642: root->totalSizeOfCommittedPages -=
page->bucket->slotSize;
+ kSystemPageSize as per above?

[Tom Sepez, 2014-10-15 22:07:33]

On 2014/10/15 22:06:17, Chris Evans wrote:
> Hmm. I'm actually leaning to just getting rid of the "partitionFull" concept.
I
> can't think of any critical reason why we have this logic. Tom, can you
SGTM.

[Tom Sepez, 2014-10-15 22:15:43]

Hold it.  What about blink strings overflowing when there are more that 2GB of
empty strings mallocd, since they stole some bits away from the 32-bit refcount
?

[Chris Evans, 2014-10-15 22:22:25]

On 2014/10/15 22:15:43, Tom Sepez wrote:
> Hold it.  What about blink strings overflowing when there are more that 2GB of
> empty strings mallocd, since they stole some bits away from the 32-bit
refcount
> ?

We're talking about refcount overflows?

I think that's generally covered by address space limits? There's a 16GB limit
on Linux x64 for example. Assuming the smallest possible reference size of a
single pointer, the largest refcount attainable is 16GB (less binary mappings) /
sizeof(void*) == 2 billion or so, certainly < INT_MAX.

How many bits of refcount got swiped?

[Tom Sepez, 2014-10-15 22:31:16]

> How many bits of refcount got swiped?
I'm looking and not finding.  Lemme ping abarth@

[Tom Sepez, 2014-10-15 23:13:37]

Per abarth, it was StringImpl, but we fixed the crazy.  So we're good.

[Jens Widell, 2014-10-16 11:09:53]

I've reduced this CL to just fix committed pages counting for direct mapped
allocations.

I'll upload another CL that drops the partitionFull() mechanism.

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

https://codereview.chromium.org/649583004/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:581: root->totalSizeOfCommittedPages += size;
On 2014/10/15 22:06:16, Chris Evans wrote:
> I think we might as well try and be fully accurate: the metadata page will
also
> be committed so hows about "size + kSystemPageSize" ?

Done.

https://codereview.chromium.org/649583004/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:642: root->totalSizeOfCommittedPages -=
page->bucket->slotSize;
On 2014/10/15 22:06:16, Chris Evans wrote:
> + kSystemPageSize as per above?

Done.

[Jens Widell, 2014-10-16 11:14:14]

On 2014/10/16 11:09:53, Jens Widell wrote:

> I'll upload another CL that drops the partitionFull() mechanism.

And I just found another reason to have it (of sorts): In
partitionAllocBaseShutdown(), we stack-allocate an array of super page pointers,
of size (kMaxPartitionSize / kSuperPageSize). IOW, we use the maximum partition
size as an upper limit on the number of super pages we might have.

We'll need to redesign that code a bit, I suppose.

[hiroshige, 2014-10-16 11:16:52]

> On 32-bit, we're probably about as likely to run out of
> actual address space (and hit partitionOutOfMemory()) as we were to hit
> partition full before, but things shouldn't be worse there at least.

I think the problem will still occur in 32-bit environments (it will take a
little more time to crash though).
I tested Windows 32-bit Chrome with Patch Set 1 of this CL (running on Win7
64bit).
The original textarea.html in crbug.com/419729: didn't crash.
If I modified s/i < 20000/i < 27000/: crashed.

[Jens Widell, 2014-10-16 11:20:51]

On 2014/10/16 11:16:52, hiroshige wrote:
> > On 32-bit, we're probably about as likely to run out of
> > actual address space (and hit partitionOutOfMemory()) as we were to hit
> > partition full before, but things shouldn't be worse there at least.
> 
> I think the problem will still occur in 32-bit environments (it will take a
> little more time to crash though).
> I tested Windows 32-bit Chrome with Patch Set 1 of this CL (running on Win7
> 64bit).
> The original textarea.html in crbug.com/419729: didn't crash.
> If I modified s/i < 20000/i < 27000/: crashed.

Yes, the address space "leak" (or whatever you want to refer to it as) still
exists unchanged. The only difference this patch does is to remove one limit
(our own 2 GB limit), leaving the unavoidable limit "how much address space the
OS can/will let us allocate."

That the original TC still crashes is completely expected.

[Chris Evans, 2014-10-16 22:23:02]

LGTM

Thanks for bringing this inconsistency into line.

---

It occurs to me that we can now detect leaks of direct mapped allocations, along
these lines:

1) In the shutdown code, flush the list of empty but still committed pages, by
decomitting them.

2) In the absence of any leaks of either bucketed or direct-mapped allocations,
the "total committed pages" counter will be 0. We can check for that as another
leak signal.

As a side benefit, note that the test asserts that there have not been any
leaks. This would also be effectively asserting that our "total committed page"
account is correct. I think that's an important assert to have, we'll have nasty
v8 GC interactions if we start getting the accounting wrong.

Feel free to action this in this CL, or another CL, or not at all.

[Jens Widell, 2014-10-21 09:59:46]

On 2014/10/16 22:23:02, Chris Evans wrote:
> LGTM
> 
> Thanks for bringing this inconsistency into line.
> 
> ---
> 
> It occurs to me that we can now detect leaks of direct mapped allocations,
along
> these lines:
> 
> 1) In the shutdown code, flush the list of empty but still committed pages, by
> decomitting them.
> 
> 2) In the absence of any leaks of either bucketed or direct-mapped
allocations,
> the "total committed pages" counter will be 0. We can check for that as
another
> leak signal.
> 
> As a side benefit, note that the test asserts that there have not been any
> leaks. This would also be effectively asserting that our "total committed
page"
> account is correct. I think that's an important assert to have, we'll have
nasty
> v8 GC interactions if we start getting the accounting wrong.
> 
> Feel free to action this in this CL, or another CL, or not at all.

This is a very good idea, and especially as a way to check that the committed
pages counter is accurate.

I'll look into adding this in a separate CL.

[Jens Widell, 2014-10-22 06:51:21]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-10-22 06:51:46]

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

[commit-bot: I haz the power, 2014-10-22 07:30:48]

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

[commit-bot: I haz the power, 2014-10-22 07:30:49]

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

[Jens Widell, 2014-10-22 09:22:48]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-10-22 09:23:28]

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

[commit-bot: I haz the power, 2014-10-22 11:17:44]

Committed patchset #2 (id:20001) as 184169