AFAICT, decommitting is the right thing to do.

+haraken, brucedawson

Is there any difference in the CPU cost? Removing pages from the working-set of a process has a non-trivial cost. I would *expect* that the cost for DiscardVirtualMemory is probably the same as VirtualFree, because both probably immediately remove the pages from the working set. If so then I would agree that decommitting is best.

That is, is there any known advantage of using DiscardVirtualMemory?

tl;dr: DiscardVirtualMemory probably shouldn't exist in PartitionAlloc based off of a clarification discussion I had with erikchen@. This code appears to be dead code.

===========================

MSDN is pretty clear that both DiscardVirtualMemory and VirtualAlloc(MEM_RESET) do not decommit from your address space:

DiscardVirtualMemory:
Discards the memory contents of a range of memory pages, without decommitting the memory. The contents of discarded memory is undefined and must be rewritten by the application.
https://msdn.microsoft.com/en-us/library/windows/desktop/dn781432(v=vs.85).aspx

VirtualAlloc(MEM_RESET):
Indicates that data in the memory range specified by lpAddress and dwSize is no longer of interest. The pages should not be read from or written to the paging file. However, the memory block will be used again later, so it should not be decommitted.

So in this respect, DiscardSystemPages looks like it's doing what its supposed to be doing. In fact, it points this out in its comment:
// Only committed pages should be discarded. Discarding a page does not
// decommit it, and it is valid to discard an already-discarded page.
https://cs.chromium.org/chromium/src/base/allocator/partition_allocator/page_allocator.h?q=DiscardSystemPages+package:%5Echromium$&l=99

Taking a step back, the main call chain that hits DiscardSystemPages is the following:
DecommitFreeableMemory (passes base::PartitionPurgeDecommitEmptyPages)
PartitionPurgeMemoryGeneric(..., flags include PartitionPurgeDiscardUnusedSystemPages but NOT PartitionPurgeDecommitEmptyPages)
PartitionPurgeBucket
DiscardSystemPages
PartitionPurgePage(..., discard == true)

Based off of this indexed find, given that PartitionPurgeDecommitEmptyPages is the only flag ever used (PartitionPurgeDiscardUnusedSystemPages is never specified), this code is likely dead.

I think I see a couple other places where DiscardSystemPages is used, e.g. https://cs.chromium.org/search/?q=discardsystempages+package:%5Echromium$&type=cs

So I'm not convinced it's dead code. However, I do think that it has the wrong behavior on Windows.

AFAICT, the point of DiscardSystemPages is that under certain conditions, we want to "Purge" pages. Which is to say, we want two effects:
  * Free the underlying system resources.
  * Allow re-use of the page without another syscall.

On POSIX-y systems, madvise(MADV_FREE) has this behavior. However, on Windows, it's not possible to get both effects. Either we use VirtualAlloc(MEM_RESET) [or equivalent], which gives us the page re-use without another syscall, but doesn't actually decommit the pages. This means that the physical pages of RAM can be reused, but we're not truly freeing all resources [the system still has to reserve space in the PageFile]. Decommitting the page has the desired effect on system resources, but requires another syscall before the virtual pages can be reused.

Basically, Windows does not expose the same APIs as POSIX-y systems, and DiscardSystemPages cannot be implemented across all platforms. 

The only interesting call in the search results from #5 is
src/third_party/WebKit/Source/platform/heap/HeapPage.cpp

The others are from copies of partition allocator scattered through the Chromium repository.

That's only called if
MemoryCoordinator::IsLowEndDevice(), which is gated by a finch trial that expired in 2015.

https://cs.chromium.org/search/?q=DiscardPages+package:%5Echromium$&type=cs

@robliao: did you feel like ripping the code out or should I?

I'm going to be OOO Thursday and Friday, but happy to do it when I get back. Feel free to do it in the meantime. Just send this bug my way if you don't get to it before I get back.

Given this appears to be dead, I'm adjusting the priority to 3.

Taking a stab at it here:

https://chromium-review.googlesource.com/c/514346/

Per haraken@ in https://chromium-review.googlesource.com/c/514346/#message-fd21805e0aea7e11a30ae221ed5af80d0ee7dd4d

"""
PartitionAllocDiscardSystemPages are still used in key scenarios.

Before we ship Memory Coordinator, it is called by RenderThreadImpl::ReleaseFreeMemory, which is called when the pressure signal is dispatched, when Purge+Throttle runs etc.

After we ship Memory Coordinator, it will be called by Memory Coordinator.

Would there be any option to fix the issue on Win?
"""

To recap, the current behavior is on windows, we discard using DiscardVirtualMemory and/or VirtualAlloc(MEM_RESET). Both effectively drop the page from memory, but doesn't let go of the reserved swap.

I'm sure what the expected behavior from Purge+Throttle, etc., is under this signal. If *think* it's not sufficient to let go of the physical page. We want to decommit it fully as listed here:

   https://docs.google.com/document/d/1thLjq-PYoKWaM_ODDZjojr9VRG6uGnxeL86EaSzYL5k/edit#

If so, then this code should be changed to use VirtualAlloc(MEM_DECOMMIT).

That being said reading https://docs.google.com/document/d/1thLjq-PYoKWaM_ODDZjojr9VRG6uGnxeL86EaSzYL5k/edit# (the link to the raw results seem to be broken), it looks like the PartitionAlloc purging was not a major benefit.  Do we want to keep this feature?

(also, now that I look at my CL again, I deleted more than I should have...will send another patch)

Read through the code more and decided I actually was removing the right chunk.

As for purging ParitionAlloc, I just quickly went through a memory dump trace file from my windows machine, and if I'm reading it right, I think we'd only save a few k on my run?

Would be curious to see the original data.

If we want to decommit the page, the problem is we'd then actually have to perform some moderate surgery on PartitionAlloc because the semantics changes significantly. We'd actually be resizing the allocated chunk and not just "discarding" it.

I think this saved a couple of MB in some websites.

Regardless it seems important to figure out what we should do here (rather than simply removing the purging code) since other allocators (e.g., Oilpan) are doing a similar thing.

> If we want to decommit the page, the problem is we'd then actually have to perform some moderate surgery on PartitionAlloc because the semantics changes significantly. We'd actually be resizing the allocated chunk and not just "discarding" it.

Would you elaborate on what kind of surgery is needed? What's a common way to "discard physical memory" on Win? Also is this a specific issue to Win? (i.e., does the current code work for other platforms?)

> Would you elaborate on what kind of surgery is needed? What's a common way to 
> "discard physical memory" on Win? Also is this a specific issue to Win?
> (i.e., does the current code work for other platforms?)

The code *does* work for other platforms so we could make it a noop in windows, but let's me start from the top since I think you and I are talking with different understandings of how this works.

PartitionAlloc requests memory from the system using SystemAllocPages():
   https://cs.chromium.org/chromium/src/base/allocator/partition_allocator/page_allocator.cc?q=SystemAllocPages&l=49

In Unix-ish systems (Linux/Mac/Android/Cros) this is done via mmap(MAP_ANONYMOUS | MAP_PRIVATE). In Windows this done via VirtualAlloc(MEM_RESERVE | MEM_COMMIT). This has 2 different effects.

Unixish:
  - Virtual Address space is reserved.
  - *Nothing* is allocated or reserved until pages are touched.
  - When a page is touched, it is faulted into physical ram (aka RSS) and then may bay be written out to swap.

Windows:
  - Virtual Address space is reserved.
  - Space in the Pagefile is reserved immediately an cannot be given back unless the memory is decommited via VirtualAlloc(MEM_DECOMMIT).
  - When a page is touched, it is faulted into physical ram (aka WorkingSet) and then may be written out the the previously reserved space.


The critical difference here is that on Windows, system resources are occupied regardless of whether or not memory is touched. On Unixish systems, the "touch" is the trigger for backing with real resources (ram or swap). Thus, theoretically, on Windows, the touching of committed memory cannot fail (hence the term committed) whereas on UNIXish systems, it can fail (probably raising a SIGBUS? I tried briefly to test, but crashed my system probably has crashed since OS processes...like the system logger...can't allocate either).

This changes the meaning of "discard" quite significantly on both systems which is where the problem shows up.  For discard, 

On Unix-ish systems, madvise(address, length, MADV_FREE_REUSABLE) and similar will return to the original state of 

  - Virtual Address reserved
  - no physical ram or swap allocated.
  - When an application retouches the address range, the OS attempts to allocate ram or swap back as needed.

This makes discarding fully transparent to the higher applications.

On Windows, DiscardVirtualMemory() and VirtualAlloc(MEM_RESET) both will return to the original state of:
  - Virtual Address reserved
  - removes the physical ram page (without causing any swap-in/swap-outs)
  - it RETAINS the Pagefile reservation.

Thus, the "discarded memory" is still counted against the "total commit charge" and can prevent other programs from loading because windows will not allow overcommit of memory globally. Note that if you viewed the task manager numbers or the Memory.Total2 family of metrics, you will see a reduction for windows still since those do not count swap usage. This number is misleading since Chrome will still be using resources that cannot be used by other programs.

That's the current state of the world.  If we wanted to make Windows behave like the Unix-ish OSes, then we would need to "decommit" the memory which will retain the VM mapping but release the ram/swap reservations.  Unfortunately, unlike Unixish systems, touching decommitted memory will give an Access Violation instead of faulting in a new page. You have to do a VirtualAlloc(MEM_COMMIT) on the region before using it again.

====

Bringing that all the way back to this "purge" code, there seems to be 2 pieces of purge code. One in PartitonAlloc proper and one in Oilpan (which is using PartitionAlloc only as a PageHeap).

In both, we seem to be walking through the allocation metadata, looking for parts of the allocation extent that are not used, and then "discarding" them to regain some space. On Unix, this makes sense, but on Windows, this only affects the working set. If we were to change the code to use MEM_DECOMMIT in windows, then we'd have to actually remove that VirtualAddress region from use (don't add to the freelist in Oilpan, update the heap metadata in PartitionAlloc proper to make it look like the region was smaller).

It's doable, but I'm wondering if it is worth the complexity for the memory gained?

Note that confusingly, there's a comment here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/HeapPage.cpp?type=cs&l=1355 suggesting that the "discard" will cause extra page faults.  I think this is opposite of what is actually happening. The discards should not cause any swapping (and in fact are designed as a hint to avoid swapping). It should be the walking of the heap metadata that's causing the swapping.

BTW, I think there are 2 different bits of code here to consider:

(1) PartitionAlloc's "discard" on purge:
used here:
https://cs.chromium.org/chromium/src/base/allocator/partition_allocator/partition_alloc.cc?type=cs&l=1076


(2) Oilpan's "discarding" which is currently behind a flag:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/HeapPage.cpp?l=1358
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/HeapPage.cpp?l=1370

@haraken: I think your concern is mostly with the Oilpan entrypoints to this code? Is that correct? Or do you also think the PartitionAlloc::PartitionPurgePage() is worth preserving? 

Last comment before all y'all wake up in Tokyo...

I just reread https://docs.google.com/document/d/1thLjq-PYoKWaM_ODDZjojr9VRG6uGnxeL86EaSzYL5k/edit# and either someone fixed the result links (thanks if you did!) or I was looking at the wrong doc earlier. Regardless, I'm not sure this CL would significantly effect those results?

The key change is this I think:
  https://codereview.chromium.org/1707623002/diff/20001/third_party/WebKit/Source/web/WebMemoryPressureListener.cpp

which calls WTF::Partitions::decommitFreeableMemory(); which is a different codepath where PartitonAlloc drops empty pages that it's keeping around for performance.  This is different from the "purge discardable" implementation.

It *might* actually worsen the Oilpan numbers. I'd have to dig into whether or not the Oilpan metrics count the memory in its freelists...

Thanks for the clarification!

In general, PageAllocator's DiscardSystemPages is an effective way to "discard" unused system pages from PartitionAlloc and Oilpan on Unix. It is used in Oilpan (the IsLowEndDevice flag is already enabled on certain devices), existing MemoryPressureListeners and the tab purging. It will be used in Memory Coordinator once it's enabled. When we introduced the discarding logic, we confirmed that it drops several MB in some websites. So I don't think it would make sense to remove the code.

Would it be an option to preserve DiscardSystemPages on Unix but make it noop on Win?

Out of curiosity, let me ask two more questions:

- Is there any problem in preserving DiscardSystemPages on Unix? (I'm assuming the answer is no.)

- Is there any problem in preserving DiscardSystemPages on Win? I now understand that there is no merit in preserving DiscardSystemPages. I'm curious if it's a harmful thing or not.

> When we introduced the discarding logic, we confirmed that it drops several MB in some websites. So I don't think it would make sense to remove the code.

You citing this previously as well but I think #15 shows that it is not necessarily true?

Looking at the cited test CL, and it does NOT actually exercise this code path in PartitionAlloc. What the test did was (accidentally?) remove the pre-step of decommitting empty pages before dumping stats. The delta is almost certainly caused by the fact that your on pressure listener called `decommitFreeableMemory()` which does NOT touch this PartitionAlloc discardable memory path. If you look at the data in 
  https://docs.google.com/spreadsheets/d/1Y9rsxLo_0vkNx5SyCPQkyMbb5aSo3aSBD7PktJrB5tg/edit#gid=0

The "discardable" column is actually the stat that tracks the amount of memory that would have been "discarded" on unix.  It's usually 0, and the delta is at most ~100k between the m0 and m1 timestamps.

By the above, I think the partition alloc discarding code is not worth maintaining, or am I missing something?


For the Oilpan's use of "discard", the test data remains valid, but the impact is way smaller. The top level doc shows ~0.3mb savings only. That being said, the raw data in https://docs.google.com/spreadsheets/d/1ZYpVyPm59JtpmhCsZMN5aCN_Ue_rC3GjPP1yW-cQVnw/edit#gid=1826186610 doesn't match the top level doc and shows larger drops (on the order of about 1 meg sometimes if renderer is paused and purged.)

I'm not sure which number is right, but we are talking about ~1 meg in one site (Wordjournal) and ~300k in most others.

To summarize and answer your 2 questions:

The multi-meg gain seems to be from _decommitting_ empty pages in PartitionAlloc, and not discarding memory in either PartitionAlloc (which the test didn't do) or oilpan (which yields round 300kb).

- Keeping DiscardSystemPages() on Unix: No real problem.
- Keeping DiscardSystemPages() on Windows: also no harm. It *does* help with avoiding swapping (at the cost of 1 syscall).

If that sounds all right, then I think the right path is preserve DiscardSystemPages() in PageHeap, but remove the PartitionAlloc "PartitionPurgeDiscardUnusedSystemPages" feature.

Deleted comment 18 since it wasn't well thought out.

> Looking at the cited test CL, and it does NOT actually exercise this code path in PartitionAlloc. What the test did was (accidentally?) remove the pre-step of decommitting empty pages before dumping stats. The delta is almost certainly caused by the fact that your on pressure listener called `decommitFreeableMemory()` which does NOT touch this PartitionAlloc discardable memory path.

decommitFreeableMemory() is touching the discardable memory path. decommitFreeableMemory() calls PartitionPurgeMemoryGeneric(), which calls PartitionDecommitEmptyPages() (this decommits empty pages) and PartitionPurgeBucket() (this discards system pages).

However, maybe you're right that most of the reduction comes from the "decommit" and the "discard" is not doing meaningful work.

> For the Oilpan's use of "discard", the test data remains valid, but the impact is way smaller. The top level doc shows ~0.3mb savings only. That being said, the raw data in https://docs.google.com/spreadsheets/d/1ZYpVyPm59JtpmhCsZMN5aCN_Ue_rC3GjPP1yW-cQVnw/edit#gid=1826186610 doesn't match the top level doc and shows larger drops (on the order of about 1 meg sometimes if renderer is paused and purged.)

We had to introduce the "discard" logic to Oilpan to fix a significant memory regression observed in System Health Benchmarks (https://codereview.chromium.org/1666083002/).

Overall I agree with you that the discard mechanism might not be doing as meaningful work as we expect. However, I'm not sure if it's a good idea to spend lots of efforts to prove that it is not doing meaningful work and thus we should remove it. Unless it is not causing any harm, I'd prefer just keeping the code as is.

(FWIW tasak@ is now investigating the heap fragmentation in PartitionAlloc and measuring how much the decommit & discard can drop in real-world websites.)

> decommitFreeableMemory() is touching the discardable memory path. decommitFreeableMemory() calls PartitionPurgeMemoryGeneric(), which calls P> artitionDecommitEmptyPages() (this decommits empty pages) and PartitionPurgeBucket() (this discards system pages).

Ah, I think this is where we the confusion is.  decommitFreeableMemory only ever calls PartitionPurgeMemoryGeneric() with base::PartitionPurgeDecommitEmptyPages.  Thus this line is always false:

https://cs.chromium.org/chromium/src/base/allocator/partition_allocator/partition_alloc.cc?type=cs&q=PartitionPurgeMemoryGeneric+package:%5Echromium$&l=1226

And PartitionPurgeBucket() is never called.

I really do think this code is dead and wasn't part of the test earlier.

As for not causing harm...this specific piece of code is actually causing some passive harm. It's confused the understanding of at least 3 engineers over the last 6 months (you, me, erikchen) and spawned at least 2-3 weeks of work trying to understand how "discard" in windows affects counters because we all thought this might be a significant factor in miscounting of our metrics.

I know it feels like we're sinking a lot of time into this, but I feel like the complexity has cost enough engineering time to be worth reducing.

As for heap fragmentation, that's a good thing to look at but I think that's an unrelated point. On Unix, decommit is the right thing to do there as discard is the same exact thing. On windows, the discard concept isn't going to help you much if at all.

> Ah, I think this is where we the confusion is.  decommitFreeableMemory only ever calls PartitionPurgeMemoryGeneric() with base::PartitionPurgeDecommitEmptyPages.  Thus this line is always false: [...]

I would +1 that analysis as it agreed with Comment #3 above.

Ah, thanks! Now I got it. Sorry for the confusion.

tasak@: Would you quickly check if enabling base::PartitionPurgeDecommitEmptyPages and seeing if it reduces a significant amount of memory?

If yes, we should keep it but otherwise remove the code.

ajwong@: Does that sound reasonable?

Sounds reasonable to me!  haraken@: thanks for walking through all this...you were right in comment #12 that I didn't fully understand the code I was removing. This was very educational for me.

tasak@: FYI, I *think* you may already have this data actually from the previous tests.  While the test code did not actually discard memory, the stats dumping code included this line:

https://cs.chromium.org/chromium/src/third_party/pdfium/third_party/base/allocator/partition_allocator/partition_alloc.cc?q=discardable_bytes+package:%5Echromium$&l=1254

which *I think* comes out as the value of the "discardable" column in the raw data and is actually exactly the amount of memory that would have been "discarded" had you used the flag.

However, as this thread has shown, this code is confusing enough that I only trust my read of that 80%.

When you rerun the test, maybe double check that the "discardable" number is in line with whatever results you get as a sanity check.

Let me know what you find. I'll clean up the CL to reflect this discussion on monday and ping all y'all again for review.

I tried base::PartitionPurgeDiscardUnusedSystemPages... So I saw DCHECK failure inside PartitionAlloc.
I'm now investigating why DCHECK fails.

I enabled base::PartitionPurgeDiscardUnusedSystemPages (i.e. applying partitions.cpp.diff) after applying https://codereview.chromium.org/2921283002/, and ran system_health benchmark on linux (Z840 workstation).

The attached results.html shows the result. "0" says memory usage of original chrome (refs/heads/master@{#477155}), and "1" says memory usage of the modified chrome.

Comparing the memory usages, PartitionPurgeDiscardUnusedSystemPages seems to reduce PartitionAlloc memory usage by +2~+4%, (i.e. 200KB~500KB). PrivateDirty, ResidentSetBytes, and PSS look almost the same (=not affected).

I think, currently it is a little difficult to reduce PartitionAlloc memory usage by 200KB~500KB.
So I would like to enable PartitionPurgeDiscardUnusedSystemPages when decommitting freeable memory.
 

Deleted: result.html 860 KB

result.html 860 KB View Download

Deleted: partitions.cpp.diff 1.5 KB

partitions.cpp.diff 1.5 KB Download

I found a mistake of patchset 1:  https://codereview.chromium.org/2921283002/
(so base_unittests failed).
I fixed the issue and now I'm trying to obtain new data.

Thanks @tasak!

Can you explain the discrepancy between the "discardable" number which and the PartitionAlloc numbers in the result?

By my reading of the code, the "discardable" count, should be _exactly_ the number of bytes we pass into the DiscardSystemPages() function. That we're seeing a few orders of magnitude difference there (discardable is in bytes, partition alloc usages is in kbytes) doesn't make sense.

I reran system_health benchmark and obtained new data. I also added partition_alloc/discardable_size.

I think, the "discardable_size" is _exact_ number reduced by PartitionPurgeDiscardUnusedSystemPages.

Deleted: result.html 973 KB

result.html 973 KB View Download

Ah hah! So discardable is NOT the same as partition_alloc/discardable_size. Now the numbers do match up in showing about a 200k decrease. Thanks for verifying.

Agreed that this seems like it's worth keeping around and actually enabling.

Changing the bug title to reflect this.

@tasak are you writing the CL to enable it or did you want me to?