PartitionAlloc: add a discardSystemPages() hint API.

PartitionAlloc: add a discardSystemPages() hint API.

We will use this API to mark unneeded pages inside a more powerful variant
of partitionPurgeMemoryGeneric().

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

[Chris Evans, 2015-06-18 16:37:12]

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

[haraken, 2015-06-18 18:15:40]

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

https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.cp...
Source/wtf/PageAllocator.cpp:225: {

Shall we add ASSERT(!(len & kSystemPageOffsetMask)) ?

https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h
File Source/wtf/PageAllocator.h (right):

https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h#...
Source/wtf/PageAllocator.h:116: // decommit it, and it is valid to discard an
already-discarded page.

I'm a bit confused with "Discarding a page does not decommit it" -- it looks
like you're calling decommitSystemPages in discardSystemPages.

https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h#...
Source/wtf/PageAllocator.h:117: // A read or write to a discarded page will not
fault.

Given that you call decommitSystemPages, I guess a read or write to the page can
fault.

https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h#...
Source/wtf/PageAllocator.h:120: // Writing to a discarded page is the only
guaranteed way to undo the effect of

"undo the effect of discarding it" might be a bit misleading. It's possible that
the page is already discarded and then it can return a page of zeros.

[Chris Evans, 2015-06-18 18:28:27]

On 2015/06/18 18:15:40, haraken wrote:
> https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.cpp
> File Source/wtf/PageAllocator.cpp (right):
> 
>
https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.cp...
> Source/wtf/PageAllocator.cpp:225: {
> 
> Shall we add ASSERT(!(len & kSystemPageOffsetMask)) ?

Done.

> 
> https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h
> File Source/wtf/PageAllocator.h (right):
> 
>
https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h#...
> Source/wtf/PageAllocator.h:116: // decommit it, and it is valid to discard an
> already-discarded page.
> 
> I'm a bit confused with "Discarding a page does not decommit it" -- it looks
> like you're calling decommitSystemPages in discardSystemPages.

Ah, that's an implementation detail. On Linux, discard and decommit are the
same.
I added a comment, I wonder if it's clear enough now?

> 
>
https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h#...
> Source/wtf/PageAllocator.h:117: // A read or write to a discarded page will
not
> fault.
> 
> Given that you call decommitSystemPages, I guess a read or write to the page
can
> fault.

"decommit" on Linux leads to pages that get replaced with zeroed pages when
touched,
so the "discard" contract will be honored. No fault.
On POSIX, we use MADV_FREE which meets the written contracts for both "decommit"
and
"discard". I tried to cover this in the comment too.

If / when I implement discard for Windows, the implementation for "decommit" vs.
"discard" would be different.

> 
>
https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h#...
> Source/wtf/PageAllocator.h:120: // Writing to a discarded page is the only
> guaranteed way to undo the effect of
> 
> "undo the effect of discarding it" might be a bit misleading. It's possible
that
> the page is already discarded and then it can return a page of zeros.

Agreed. Clarified it a bit.

[haraken, 2015-06-18 18:33:39]

On 2015/06/18 18:28:27, Chris Evans wrote:
> On 2015/06/18 18:15:40, haraken wrote:
> >
https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.cpp
> > File Source/wtf/PageAllocator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.cp...
> > Source/wtf/PageAllocator.cpp:225: {
> > 
> > Shall we add ASSERT(!(len & kSystemPageOffsetMask)) ?
> 
> Done.
> 
> > 
> > https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h
> > File Source/wtf/PageAllocator.h (right):
> > 
> >
>
https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h#...
> > Source/wtf/PageAllocator.h:116: // decommit it, and it is valid to discard
an
> > already-discarded page.
> > 
> > I'm a bit confused with "Discarding a page does not decommit it" -- it looks
> > like you're calling decommitSystemPages in discardSystemPages.
> 
> Ah, that's an implementation detail. On Linux, discard and decommit are the
> same.
> I added a comment, I wonder if it's clear enough now?
> 
> > 
> >
>
https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h#...
> > Source/wtf/PageAllocator.h:117: // A read or write to a discarded page will
> not
> > fault.
> > 
> > Given that you call decommitSystemPages, I guess a read or write to the page
> can
> > fault.
> 
> "decommit" on Linux leads to pages that get replaced with zeroed pages when
> touched,
> so the "discard" contract will be honored. No fault.
> On POSIX, we use MADV_FREE which meets the written contracts for both
"decommit"
> and
> "discard". I tried to cover this in the comment too.
> 
> If / when I implement discard for Windows, the implementation for "decommit"
vs.
> "discard" would be different.
> 
> > 
> >
>
https://codereview.chromium.org/1184483007/diff/1/Source/wtf/PageAllocator.h#...
> > Source/wtf/PageAllocator.h:120: // Writing to a discarded page is the only
> > guaranteed way to undo the effect of
> > 
> > "undo the effect of discarding it" might be a bit misleading. It's possible
> that
> > the page is already discarded and then it can return a page of zeros.
> 
> Agreed. Clarified it a bit.

Thakns, much clearer :) LGTM

[Chris Evans, 2015-06-18 18:33:55]

The CQ bit was checked by cevans@chromium.org

[commit-bot: I haz the power, 2015-06-18 18:34:28]

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

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

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