Yeah, that's interesting. But I'm not sure if it's a good idea to reserve a big chunk of address space in PartitionAlloc since it prevents other allocators from allocating big memory...

Maybe what we really need would be a process-wide address-space allocator.

jschuh@ Can you route this to an owner? :)

Palmer and I can talk about this when he gets back, and we'll see if there's tweaks we're comfortable making on 32-bit.

 Issue 667242  has been merged into this issue.

Seems to be a problem specific to 32-bit platforms. In email, we zeroing in on a potentially simple fix.

 Issue 672884  has been merged into this issue.

Any updates on #c6 and the nature of that fix ?

#10: Yes, we think we can have the allocator reserve a 512 MiB region right away, and satisfy smaller allocations from elsewhere in the heap. Only when those smaller allocations fail will we start "nibbling" at the 512 MiB region. So games, et c. that actually use the large region early in their lifetime stand a chance of getting it.

I'll have a stab at this.

Would it be possible to write a brief design document before start implementing?

- The overall idea is to improve page_allocator.cc and use it for all mmap calls in Chromium (https://cs.chromium.org/chromium/src/base/allocator/partition_allocator/page_allocator.cc?q=page_allocator.cc&sq=package:chromium&dr).

- Remember that V8 already has a platform-specific page allocator (https://cs.chromium.org/chromium/src/v8/src/base/platform/platform-posix.cc?q=munmap+file:%5Esrc/v8/src/+package:%5Echromium$&dr=C&l=175). We need to merge it into page_allocator.cc.

- Regarding the security for ArrayBufferContents, it doesn't necessarily need to be hardened at the layer of page_allocator.cc. Alternately, you can create a separate partition for ArrayBufferContents in PartitionAlloc (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/allocator/Partitions.h?q=partitions&dr=CSs&l=127). That way you can guarantee that addresses that have been used for allocating ArrayBufferContents will never be reused for other objects. Then you don't need to implement something complicated in page_allocator.cc.

- The current page_allocator.cc has a property that addresses returned by mmap are well randomized. If we pre-allocate a large address space, we will lose the property. We need to check in the security team if it's okay.

Absolutely - design doc in progress.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd3f72e7cc1ad766a8357728a20f2ffdc8359f05

commit cd3f72e7cc1ad766a8357728a20f2ffdc8359f05
Author: haraken <haraken@chromium.org>
Date: Thu Feb 09 06:27:02 2017

Create a dedicated partition for array buffers

ArrayBuffers have a high risk where their length and/or contents are
exploited from user scripts. Thus we don't want to mix ArrayBuffers
and other objects that may contain pointers in the same partition.

TBR=primiano@chromium.org
BUG=634547

Review-Url: https://codereview.chromium.org/2682943002
Cr-Commit-Position: refs/heads/master@{#449226}

[modify] https://crrev.com/cd3f72e7cc1ad766a8357728a20f2ffdc8359f05/base/trace_event/memory_infra_background_whitelist.cc
[modify] https://crrev.com/cd3f72e7cc1ad766a8357728a20f2ffdc8359f05/third_party/WebKit/Source/wtf/allocator/Partitions.cpp
[modify] https://crrev.com/cd3f72e7cc1ad766a8357728a20f2ffdc8359f05/third_party/WebKit/Source/wtf/allocator/Partitions.h
[modify] https://crrev.com/cd3f72e7cc1ad766a8357728a20f2ffdc8359f05/third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp

Note that https://codereview.chromium.org/2682943002 _appears_ to have stopped the test case from crashing :)

Test case -> https://chromium-tests-158800.appspot.com/ (from https://bugs.chromium.org/p/chromium/issues/detail?id=672884)

Bumping this in priority, and reassigning.
Note, the test case was fixed by separating the partition, but the OOMs continue.
This isn't surprising as the test case just did progressively larger allocations, but does not simulate a case where other allocations have absorbed all of the contiguous space.

Do we need a new, smaller test case than this one? https://s3.amazonaws.com/mozilla-games/emunittest-32bit-test/index.html

Starting to work on this. A few thoughts:

#13: I agree we should try to route all renderer allocators through an improved page_allocator.cc. The V8 allocator (base::OS::Allocate) appears to be used only by Crankshaft, which has just been removed from V8. There are several other allocators in Chrome that may have to be modified. This search turns up a bunch of calls to the Win32 VirtualAlloc function (outside third_party):

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

#17: The test case (#16) only allocates a single 512 MB array buffer. I'm not sure why creating a separate partition for array buffers (#15) fixed the test. Starting up a renderer process appears to cause 10-20 1-2 MB sized allocations (calls to base::AllocPages) at randomized locations. Maybe we got lucky?

Work to be done:

1) Scope improvements needed for page_allocator.cc so it is suitable for all allocators used in the renderer process.
2) Modify renderer allocators to use AllocPages and FreePages. We have to make sure grabbing a big chunk of address space doesn't starve out some other part of Chrome.
3) Modify page_allocator.cc to maintain a large (512 MB?) block of contiguous memory. (This would only be for 32 bit Windows.)

In related work, it looks like page_allocator skips ASLR for 32 bit systems:

https://bugs.chromium.org/p/chromium/issues/detail?id=394591
https://codereview.chromium.org/1401483002

Looking closer, v8::base::OS::Allocate looks like it's used for code generation outside of Crankshaft.

After offline discussion with jschuh@ - a "strategic" patch that just modifies page-allocator.cc may be sufficient. It would reserve a single large block (512 MB? 256MB?) of addr space at startup and free it after some amount of partition allocation activity.

haraken@ WDYT?

#23: I'd say it would be too hacky. I guess there would be many cases that won't work with the hack.

If you want to solve the problem, the approach you proposed in #20 would be a right way to go.

haraken@ Are you suggesting routing all allocators through page_allocator.cc is required before you're comfortable with the reservation?

When we tried to get this issue sorted previously, I had the sense knocking on the door of all teams not routed through page_allocator would require a pretty broad mandate.

If we need to get that underway, I'm eager to begin reaching out, as this continues to be Unity's #1 concern with content now deployed through asm.js.
This also shows up in UMA fairly prominently as something hit in practice.

> haraken@ Are you suggesting routing all allocators through page_allocator.cc is required before you're comfortable with the reservation?

Yes. I don't think the hack described in #23 would solve the problem.

I think that the only allocator you need to fix would be V8. PartitionAlloc and Oilpan are already using page_allocator.cc. malloc (e.g., tcmalloc) is not using page_allocator.cc but there's no easy way to replace it (also I don't think that malloc is the culprit of our current problem).

So the action items are just:

a) Implement the address space reservation in page_allocator.cc.
b) Make V8 use page_allocator.cc.

right? Regarding b), I've already talked with the V8 team and Hannes is happy about it.

Thanks haraken@. That helps reduce the scope of the problem.

V8 has a lot of platform specific code for memory allocation, and lots of call sites, mostly in test code but also some code generation. It seems like a big project to refactor to use an external allocator.

Currently we pass the ArrayBuffer partition allocator when creating a V8 Isolate, which V8 uses for creating ArrayBuffers. Would it be reasonable to pass another partition allocator to V8 in the same way for heap allocations? Would the FastMalloc partition be appropriate, or would an entirely new partition be needed?

+hpayer

Hannes is ooo for a while.

> Would it be reasonable to pass another partition allocator to V8 in the same way for heap allocations? Would the FastMalloc partition be appropriate, or would an entirely new partition be needed?

I don't think this is an option because PartitionAlloc is optimized for single-threaded allocations while V8 needs multi-threaded allocations. The V8 team is concerned about using PartitionAlloc.

> V8 has a lot of platform specific code for memory allocation, and lots of call sites, mostly in test code but also some code generation. It seems like a big project to refactor to use an external allocator.

Why is it so hard? You just need to replace mmap / munmap in platform-macos.cc, right?

https://cs.chromium.org/chromium/src/v8/src/base/platform/platform-macos.cc

Whoever works on this please come up with a design doc first.

For the backend of the GC we do not just require mmap but also proper hinting for ASLR, alignment restrictions on all platforms, ways of changing protection levels, and a way a shrinking reservations.

Unless those features are present there's no way to switch the GC. That's basically the interface on VirtualMemory.

Are we still talking about allocating a 512MB contiguous area on 32bit?

+1 to design doc. Changing V8 to use PartitionAlloc without regressing performance, memory, and ASLR seems like a huge effort to me.

Issues to consider in addition to the ones listed in comment #29:
1) Fragmentation: PartitionAlloc uses 2MB pages, V8 uses 512KB pages. Long running app can end up with many half-filled 2MB pages.
2) Page headers: both ParitionAlloc and V8 use the area at the start of the page to store meta information.
3) Fast concurrent allocation/freeing mentioned in #28.
4) Reserving virtual address range without committing it. This is needed for V8 code range on 64-bit platforms.
5) V8 is used not only in Chrome, but also in NodeJS and other embedders.

These are issues that come to mind after few minutes of thinking. I am sure there a lot more issues that we are not seeing currently.

+1 to design doc.

As for comment #26, I don't think that V8 needs to move wholesale to use the page allocator. It should be sufficient just to change the embedder's implementation of the array buffer allocator.

The only problem remains is the OOM situation. If another allocator that is not based on page allocator fails to allocate, the page allocator should be notified to give up its large reservation and then the original allocator should try again. That could be done a variety of ways, but doesn't fundamentally require a rework of V8's allocation AFAICT.

I agree that we should have a design doc.

#31: We are not planning to change V8 to use PartitionAlloc. We are just planning to change V8 to use page_allocator.cc (i.e., mmap and munmap).

#32: I'm a bit confused. Array buffers are already allocated by PartitionAlloc, which is using page_allocator.cc, right?

#33: My point was that the only V8 change that I can see being absolutely necessary would be the OOM dance: fail/ask-partition-alloc-to-release-large-reservation/retry. Changing V8 to use page allocator isn't a prerequisite (i.e. #26b).

From V8's perspective, array buffers are simply allocated through an embedder-provided call. AFAIK this implementation in Blink simply uses PartitionAlloc, so if PartitionAlloc handles the large allocations using a pre-reservation, then there is nothing to be done on the V8 side.

#34 (titzer): Ah, that's a good point. Yeah that might be sufficient at the moment.

#29 (mlippautz): page_allocator.cc already has the functionality (ASLR, alignment restrictions, changing protection). In theory it wouldn't be that hard to unify the page allocator between V8 and others. (If page_allocator.cc lacks some functionality, we can add it.)

Having different page allocators leads to a bug like  issue 738926  and  issue 738925 . In general it would be nice to unify the page allocators.

#35: Yep, if there's feature parity and somebody willing to work on that I don't see a reason why we wouldn't be able to do that. Keep in mind though that it needs to play nicely with other embedders, so libplatform somehow needs to depend on the allocator etc etc etc :) I would assume that this is not just as simple as replacing one or two functions. 

I guess making PartitionAlloc available to V8/platform in first place is a good starter.

As Ben highlighted, for the wasm features we would get away with a callback on platform that is used to try and free up other reservations before OOM-ing after another retry.

Sorry for my confusing comment. The proposal was only to use page_allocator instead of mmap/VirtualAlloc in base/platform.

titzer@, I like your idea #34. That looks like a small change.  I'll produce a design document, but a brief sketch is that the embedder will provide a function to be called when a heap allocation fails. This callback would be passed into V8 when creating the isolate. Then memory can be reserved by the embedder (in chrome, for WASM purposes, early in Blink startup, and only on 32 bit Windows for now) and either used for a suitably large ArrayBuffer, or released when needed by PartitionAlloc or V8.

Design Document created:

https://docs.google.com/document/d/1WQTr2moZFnovwTWyoxWtQuQc8fN2U37HNZykrhw1TuA/edit#heading=h.y7988bfnn8ly

Would you make the design doc public? I don't have a permission to read it.

Implementing the address space coordinator only to the array buffer partition sounds a bit too hacky. I'd prefer implementing the address space coordinator in page_allocator.cc (instead of PartitionAlloc) and expose the callback to clear the reservation to V8.

How hard would it be to unify page_allocator.cc with V8's mmap/munmap allocator? At a first glance they are doing similar things. I'm not sure if implementing titzer's approach is really easier than unifying the page allocators.

Using page allocator in V8/libplatform would first require making it available to V8. IS this already possible, i.e., are DEPS and gn targets available for PartitionAlloc?

PartitionAlloc is in //base/. I think that //v8/ is not allowed to depend on //base/, right?

Then will we probably need to inject function pointers of the page allocator functions when v8 is instantiated? (just like we're planning to inject a function pointer of the OOM callback to v8 in titzer's proposal)

This is important for multiple WASM/asm.js use cases. Bumping priority.

#41: Is that the case, that v8 can't depend on base? If so, it's another point in favor of making PA completely independent, which is something I've been thinking about on and off. Maybe.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/725a82c7aebab85c75452cd54c4925432c5deec9

commit 725a82c7aebab85c75452cd54c4925432c5deec9
Author: Bill Budge <bbudge@chromium.org>
Date: Mon Aug 14 06:19:44 2017

Add address space reservation functions to base page_allocator.

- Adds ReserveAddressSpace function, which allocates a global memory
  reserve.

- Adds ReleaseReservation function, which releases any reservation.

- Modifies AllocPages to invoke ReleaseReservation if an allocation
  fails, and to retry the allocation.

Bug: chromium:634547
Change-Id: Iaf661ac80f63ae4eb2280349ca1de933f2baea11
Reviewed-on: https://chromium-review.googlesource.com/604377
Commit-Queue: Bill Budge <bbudge@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494016}
[modify] https://crrev.com/725a82c7aebab85c75452cd54c4925432c5deec9/base/allocator/partition_allocator/page_allocator.cc
[modify] https://crrev.com/725a82c7aebab85c75452cd54c4925432c5deec9/base/allocator/partition_allocator/page_allocator.h
[modify] https://crrev.com/725a82c7aebab85c75452cd54c4925432c5deec9/base/allocator/partition_allocator/partition_alloc_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0fa7a0242b33b9cdc05c5608462b1d7ca2a4ebd6

commit 0fa7a0242b33b9cdc05c5608462b1d7ca2a4ebd6
Author: Marc Treib <treib@chromium.org>
Date: Mon Aug 14 09:18:17 2017

Revert "Add address space reservation functions to base page_allocator."

This reverts commit 725a82c7aebab85c75452cd54c4925432c5deec9.

Reason for revert: PageAllocatorTest.ReserveAddressSpace and .AllocFailure are failing on Linux Tests (dbg)(1)(32):
https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests%20%28dbg%29%281%29%2832%29

Original change's description:
> Add address space reservation functions to base page_allocator.
> 
> - Adds ReserveAddressSpace function, which allocates a global memory
>   reserve.
> 
> - Adds ReleaseReservation function, which releases any reservation.
> 
> - Modifies AllocPages to invoke ReleaseReservation if an allocation
>   fails, and to retry the allocation.
> 
> Bug: chromium:634547
> Change-Id: Iaf661ac80f63ae4eb2280349ca1de933f2baea11
> Reviewed-on: https://chromium-review.googlesource.com/604377
> Commit-Queue: Bill Budge <bbudge@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#494016}

TBR=bbudge@chromium.org,haraken@chromium.org

Change-Id: I8a11f44b719640c214f47728535778a9af419705
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:634547
Reviewed-on: https://chromium-review.googlesource.com/611991
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494023}
[modify] https://crrev.com/0fa7a0242b33b9cdc05c5608462b1d7ca2a4ebd6/base/allocator/partition_allocator/page_allocator.cc
[modify] https://crrev.com/0fa7a0242b33b9cdc05c5608462b1d7ca2a4ebd6/base/allocator/partition_allocator/page_allocator.h
[modify] https://crrev.com/0fa7a0242b33b9cdc05c5608462b1d7ca2a4ebd6/base/allocator/partition_allocator/partition_alloc_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f772bb03ce8020f7eecefc621f1e10fc161a1291

commit f772bb03ce8020f7eecefc621f1e10fc161a1291
Author: Bill Budge <bbudge@chromium.org>
Date: Tue Aug 15 00:59:19 2017

Reland "Add address space reservation functions to base page_allocator."

This is a reland of 725a82c7aebab85c75452cd54c4925432c5deec9
Original change's description:
> Add address space reservation functions to base page_allocator.
> 
> - Adds ReserveAddressSpace function, which allocates a global memory
>   reserve.
> 
> - Adds ReleaseReservation function, which releases any reservation.
> 
> - Modifies AllocPages to invoke ReleaseReservation if an allocation
>   fails, and to retry the allocation.
> 
> Bug: chromium:634547
> Change-Id: Iaf661ac80f63ae4eb2280349ca1de933f2baea11
> Reviewed-on: https://chromium-review.googlesource.com/604377
> Commit-Queue: Bill Budge <bbudge@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#494016}

Bug: chromium:634547
Change-Id: Ie070e74d882789ae1b4fc412adff04a6aefaef60
Reviewed-on: https://chromium-review.googlesource.com/614020
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494278}
[modify] https://crrev.com/f772bb03ce8020f7eecefc621f1e10fc161a1291/base/allocator/partition_allocator/address_space_randomization.h
[modify] https://crrev.com/f772bb03ce8020f7eecefc621f1e10fc161a1291/base/allocator/partition_allocator/page_allocator.cc
[modify] https://crrev.com/f772bb03ce8020f7eecefc621f1e10fc161a1291/base/allocator/partition_allocator/page_allocator.h
[modify] https://crrev.com/f772bb03ce8020f7eecefc621f1e10fc161a1291/base/allocator/partition_allocator/partition_alloc_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a78cf943cc24d02e5cbf5aeea777695baea6e14

commit 2a78cf943cc24d02e5cbf5aeea777695baea6e14
Author: Bill Budge <bbudge@chromium.org>
Date: Fri Aug 18 01:07:14 2017

[WASM] Reserve a large block of address space on 32 bit Windows.

- Reserves address space on 32 bit Windows, using
  base::ReserveAddressSpace. This will be released on the first
  failure in AllocPages.
- Implements v8::Platform::OnCriticalMemoryPressure in gin, so V8 alloc
  failures can also invoke base::ReleaseReservation.

Bug: chromium:634547
Change-Id: I9bf7b4bb406bbd138c96ed5091b5900fd8137947
Reviewed-on: https://chromium-review.googlesource.com/616340
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Brad Nelson <bradnelson@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495411}
[modify] https://crrev.com/2a78cf943cc24d02e5cbf5aeea777695baea6e14/gin/public/v8_platform.h
[modify] https://crrev.com/2a78cf943cc24d02e5cbf5aeea777695baea6e14/gin/v8_platform.cc
[modify] https://crrev.com/2a78cf943cc24d02e5cbf5aeea777695baea6e14/third_party/WebKit/Source/controller/BlinkInitializer.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3675fc4180795b6708018bd8fca2f420ffe525d

commit a3675fc4180795b6708018bd8fca2f420ffe525d
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Fri Aug 18 02:28:48 2017

Revert "[WASM] Reserve a large block of address space on 32 bit Windows."

This reverts commit 2a78cf943cc24d02e5cbf5aeea777695baea6e14.

Reason for revert: This CL breaks Android builds
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Builder%20%28dbg%29/builds/5740
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Marshmallow%2064bit%20Perf/builds/10177
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20ARMv6%20Builder/builds/5532


Original change's description:
> [WASM] Reserve a large block of address space on 32 bit Windows.
> 
> - Reserves address space on 32 bit Windows, using
>   base::ReserveAddressSpace. This will be released on the first
>   failure in AllocPages.
> - Implements v8::Platform::OnCriticalMemoryPressure in gin, so V8 alloc
>   failures can also invoke base::ReleaseReservation.
> 
> Bug: chromium:634547
> Change-Id: I9bf7b4bb406bbd138c96ed5091b5900fd8137947
> Reviewed-on: https://chromium-review.googlesource.com/616340
> Reviewed-by: Jeremy Roman <jbroman@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Chris Palmer <palmer@chromium.org>
> Reviewed-by: Brad Nelson <bradnelson@chromium.org>
> Commit-Queue: Bill Budge <bbudge@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#495411}

TBR=bradnelson@chromium.org,palmer@chromium.org,dcheng@chromium.org,bbudge@chromium.org,jschuh@chromium.org,jbroman@chromium.org,haraken@chromium.org,slangley@chromium.org

Change-Id: I5acb08fb0caa798061bd9e24209794a45925b8fc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:634547
Reviewed-on: https://chromium-review.googlesource.com/620346
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495430}
[modify] https://crrev.com/a3675fc4180795b6708018bd8fca2f420ffe525d/gin/public/v8_platform.h
[modify] https://crrev.com/a3675fc4180795b6708018bd8fca2f420ffe525d/gin/v8_platform.cc
[modify] https://crrev.com/a3675fc4180795b6708018bd8fca2f420ffe525d/third_party/WebKit/Source/controller/BlinkInitializer.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ad1fbfd6cbe7fe5a2869e39dd62473fe80f35be1

commit ad1fbfd6cbe7fe5a2869e39dd62473fe80f35be1
Author: Bill Budge <bbudge@chromium.org>
Date: Tue Aug 22 01:07:06 2017

Reland "[WASM] Reserve a large block of address space on 32 bit Windows."

This is a reland of 2a78cf943cc24d02e5cbf5aeea777695baea6e14
Original change's description:
> [WASM] Reserve a large block of address space on 32 bit Windows.
> 
> - Reserves address space on 32 bit Windows, using
>   base::ReserveAddressSpace. This will be released on the first
>   failure in AllocPages.
> - Implements v8::Platform::OnCriticalMemoryPressure in gin, so V8 alloc
>   failures can also invoke base::ReleaseReservation.
> 
> Bug: chromium:634547
> Change-Id: I9bf7b4bb406bbd138c96ed5091b5900fd8137947
> Reviewed-on: https://chromium-review.googlesource.com/616340
> Reviewed-by: Jeremy Roman <jbroman@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Chris Palmer <palmer@chromium.org>
> Reviewed-by: Brad Nelson <bradnelson@chromium.org>
> Commit-Queue: Bill Budge <bbudge@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#495411}

Bug: chromium:634547
Change-Id: I63267d9ff1ac3ec39f4c75e71d29d4d2b07ed0a3
Reviewed-on: https://chromium-review.googlesource.com/620926
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496154}
[modify] https://crrev.com/ad1fbfd6cbe7fe5a2869e39dd62473fe80f35be1/gin/public/v8_platform.h
[modify] https://crrev.com/ad1fbfd6cbe7fe5a2869e39dd62473fe80f35be1/gin/v8_platform.cc
[modify] https://crrev.com/ad1fbfd6cbe7fe5a2869e39dd62473fe80f35be1/third_party/WebKit/Source/controller/BlinkInitializer.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f25f078b34a92d81205f1bf9ceada4920c968737

commit f25f078b34a92d81205f1bf9ceada4920c968737
Author: Ian Clelland <iclelland@google.com>
Date: Tue Aug 22 19:24:28 2017

Revert "Reland "[WASM] Reserve a large block of address space on 32 bit Windows.""

This reverts commit ad1fbfd6cbe7fe5a2869e39dd62473fe80f35be1.

Reason for revert:

This is causing the Windows 7 browser test BrowserTest.ThirtyFourTabs to run out of memory periodically.

https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20%2832%29%20Tests/builds/23574
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20%2832%29%20Tests/builds/23572
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20%2832%29%20Tests/builds/23569
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20%2832%29%20Tests/builds/23566

See  https://crbug.com/757836 

Original change's description:
> Reland "[WASM] Reserve a large block of address space on 32 bit Windows."
> 
> This is a reland of 2a78cf943cc24d02e5cbf5aeea777695baea6e14
> Original change's description:
> > [WASM] Reserve a large block of address space on 32 bit Windows.
> > 
> > - Reserves address space on 32 bit Windows, using
> >   base::ReserveAddressSpace. This will be released on the first
> >   failure in AllocPages.
> > - Implements v8::Platform::OnCriticalMemoryPressure in gin, so V8 alloc
> >   failures can also invoke base::ReleaseReservation.
> > 
> > Bug: chromium:634547
> > Change-Id: I9bf7b4bb406bbd138c96ed5091b5900fd8137947
> > Reviewed-on: https://chromium-review.googlesource.com/616340
> > Reviewed-by: Jeremy Roman <jbroman@chromium.org>
> > Reviewed-by: Kentaro Hara <haraken@chromium.org>
> > Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> > Reviewed-by: Chris Palmer <palmer@chromium.org>
> > Reviewed-by: Brad Nelson <bradnelson@chromium.org>
> > Commit-Queue: Bill Budge <bbudge@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#495411}
> 
> Bug: chromium:634547
> Change-Id: I63267d9ff1ac3ec39f4c75e71d29d4d2b07ed0a3
> Reviewed-on: https://chromium-review.googlesource.com/620926
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Chris Palmer <palmer@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Jeremy Roman <jbroman@chromium.org>
> Commit-Queue: Bill Budge <bbudge@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#496154}

TBR=bradnelson@chromium.org,palmer@chromium.org,dcheng@chromium.org,bbudge@chromium.org,jbroman@chromium.org,haraken@chromium.org

Change-Id: I83a77c9b11197bb1eb4f78dadebd6f9c4b681a20
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:634547
Reviewed-on: https://chromium-review.googlesource.com/626458
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496401}
[modify] https://crrev.com/f25f078b34a92d81205f1bf9ceada4920c968737/gin/public/v8_platform.h
[modify] https://crrev.com/f25f078b34a92d81205f1bf9ceada4920c968737/gin/v8_platform.cc
[modify] https://crrev.com/f25f078b34a92d81205f1bf9ceada4920c968737/third_party/WebKit/Source/controller/BlinkInitializer.cpp

Heya... just got pointed here by ncarter@.

I think the approach being taken may not do what you expect.  The code in ReserveAddressSpace() added in
  https://chromium.googlesource.com/chromium/src/+/725a82c7aebab85c75452cd54c4925432c5deec9

calls SystemAllocPages() which eventually calls VirtualAlloc(hint, length, MEM_RESERVE | MEM_COMMIT, access_flag);

Though access_flag is PAGE_NOACCESS, VirtualAlloc() is still being passed MEM_COMMIT which forces windows to commit the memory immediately (using up space in the pagefile that other programs cannot use).  If you were looking at the Chrome task manager, you may not have seen this increase because the task manager currently shows the "working set" which is only the memory taking up phyiscal pages.

Thus, thus code is NOT just reserving memory; it is committing it in Windows which means that this code should yield a (likely) unilateral 512Mb increase in private memory usage per process in Chrome regardless of whether or not the pages are touched.

You can see that reflected in the memory counters in this proof of concept unittest:
   https://chromium-review.googlesource.com/c/chromium/src/+/627619

It would also show up on our new PrivateMemoryFootprint UMAs.

I keep meaning to write up a big warning about how the windows "committed" memory model is different from posix making an API abstraction for an X-platform address space reservation (like PageAllocator) to be perilous/impossible.  This seems to come up every few months.

In any case, if you want to do reservations for v8 on windows, you may need to skip or rethink this abstraction...

Albert, thanks for the comments. Would removing the MEM_COMMIT flag solve this problem? Since this is an internal helper function, we could arrange that to be passed in as a parameter.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/960b40deeb1fd9e889cb692f8a0f85492b84d8e6

commit 960b40deeb1fd9e889cb692f8a0f85492b84d8e6
Author: Bill Budge <bbudge@chromium.org>
Date: Tue Aug 29 01:30:03 2017

[PartitionAlloc] Reserve memory without committing on Windows.

This changes base::ReserveAddressSpace to reserve without committing.
SystemAllocPages gets a 'commit' parameter which defaults to 'true'.

Bug: chromium:634547
Change-Id: I2dc022a351273ac27310fd7c09d208ac5b9f3354
Reviewed-on: https://chromium-review.googlesource.com/639012
Commit-Queue: Bill Budge <bbudge@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497975}
[modify] https://crrev.com/960b40deeb1fd9e889cb692f8a0f85492b84d8e6/base/allocator/partition_allocator/page_allocator.cc
[modify] https://crrev.com/960b40deeb1fd9e889cb692f8a0f85492b84d8e6/base/allocator/partition_allocator/page_allocator.h
[modify] https://crrev.com/960b40deeb1fd9e889cb692f8a0f85492b84d8e6/base/allocator/partition_allocator/partition_alloc_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1b87f05bd3c9443c11b20dcbe8ae364802e8f32

commit d1b87f05bd3c9443c11b20dcbe8ae364802e8f32
Author: Bill Budge <bbudge@chromium.org>
Date: Wed Aug 30 06:44:08 2017

Reland "[WASM] Reserve a large block of address space on 32 bit Windows."

Original change's description:
> [WASM] Reserve a large block of address space on 32 bit Windows.
>
> - Reserves address space on 32 bit Windows, using
>   base::ReserveAddressSpace. This will be released on the first
>   failure in AllocPages.
> - Implements v8::Platform::OnCriticalMemoryPressure in gin, so V8 alloc
>   failures can also invoke base::ReleaseReservation.
>
> Bug: chromium:634547
> Change-Id: I9bf7b4bb406bbd138c96ed5091b5900fd8137947
> Reviewed-on: https://chromium-review.googlesource.com/616340
> Reviewed-by: Jeremy Roman <jbroman@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Chris Palmer <palmer@chromium.org>
> Reviewed-by: Brad Nelson <bradnelson@chromium.org>
> Commit-Queue: Bill Budge <bbudge@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#495411}

Change-Id: I0960ad26235ada1d727c0499a4974c216db156a3
Reviewed-on: https://chromium-review.googlesource.com/641351
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498381}
[modify] https://crrev.com/d1b87f05bd3c9443c11b20dcbe8ae364802e8f32/chrome/browser/ui/browser_browsertest.cc
[modify] https://crrev.com/d1b87f05bd3c9443c11b20dcbe8ae364802e8f32/gin/public/v8_platform.h
[modify] https://crrev.com/d1b87f05bd3c9443c11b20dcbe8ae364802e8f32/gin/v8_platform.cc
[modify] https://crrev.com/d1b87f05bd3c9443c11b20dcbe8ae364802e8f32/third_party/WebKit/Source/controller/BlinkInitializer.cpp

Oops. Somehow I wasn't on the CC list and didn't see the response.

Yes, I think removing the MEM_COMMIT will do what you want.

Though, I'm uncertain why you're calling SystemAllocPages() at all?

Rather than adding a default parameter with a value that only makes sense on one platform (what does commit=false mean on linux or mac?), why not just call VirtualAlloc() in ReserveAddressSpace()?

SystemAllocPages() is bad abstraction to have if trying to express "commit" since that doesn't have a real equivalent in any platform other than windows...

Thanks Albert for your comments. I'll refactor SystemAllocPages and its usage in page_allocator to reflect that.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c4fc90520f42c2ff1008796d26860152fec48329

commit c4fc90520f42c2ff1008796d26860152fec48329
Author: Bill Budge <bbudge@chromium.org>
Date: Wed Oct 25 14:39:02 2017

Add a histogram to track memory reservation size in renderers.

Bug: chromium:634547
Change-Id: I7bd08622b83730513c57b7e15ed97107ad57d220
Reviewed-on: https://chromium-review.googlesource.com/736788
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511452}
[modify] https://crrev.com/c4fc90520f42c2ff1008796d26860152fec48329/third_party/WebKit/Source/controller/BlinkInitializer.cpp
[modify] https://crrev.com/c4fc90520f42c2ff1008796d26860152fec48329/tools/metrics/histograms/histograms.xml

UMA from 1 day of Canary channel shows we get the full 512 MB about 98% of the time, and never get less than 256 MB. The UMA histogram is named 'Renderer4.ReservedMemory'.

Since UMA shows new ArrayBuffer allocations are failing at the same rates as before the reservation code landed, we must be releasing the reservation too early - we need the memory for a big ArrayBuffer, release it, then get hit by another big one.

I'll look into a CL to pare down the reservation.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d76b82b4c618c737011de232e58c771a6cba67d

commit 4d76b82b4c618c737011de232e58c771a6cba67d
Author: Bill Budge <bbudge@chromium.org>
Date: Wed Jan 03 21:59:53 2018

[page_allocator] Rework internal helper functions to fix some bugs.

- Makes SystemAllocPages a simple wrapper for mmap/VirtualAlloc again.
- Adds a TryAllocPages helper function to control retry logic.
  We don't want to release our reserved region if a hinted allocation
  fails where the hint is mandatory.
- Changes ReserveAddressSpace to use SystemAllocPages, to avoid dead-
  lock and to avoid releasing an existing reservation.
- On 32-bit Windows, only tries one hinted exact allocation before
  letting the OS choose base.
- Adds and correct some comments.

Bug: chromium:634547
Change-Id: I21aa695351ccd0f60e1eebe1df4df83d8d2f7ddc
Reviewed-on: https://chromium-review.googlesource.com/845118
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526831}
[modify] https://crrev.com/4d76b82b4c618c737011de232e58c771a6cba67d/base/allocator/partition_allocator/page_allocator.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e77cea9eaadf33e1d26d38d25e52ecb0be9bbb24

commit e77cea9eaadf33e1d26d38d25e52ecb0be9bbb24
Author: Bill Budge <bbudge@chromium.org>
Date: Mon Jan 08 21:53:59 2018

[page_allocator] Skip unnecessary exact size allocation tries.

- On 32 bit systems, quit after at most 2 tries at exact size allocation.
  This is a behavior change on POSIX; before, we tried 3 times, with the
  last 2 times at an address computed from |ret|.
- Add some comments describing the exact size allocation tries.
- Converts some pointer expressions to explicit comparisons with nullptr
  for consistency.

Bug: chromium:634547
Change-Id: I7c35c8f6451d06296e5404b8d611c28aa4979d10
Reviewed-on: https://chromium-review.googlesource.com/850574
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527771}
[modify] https://crrev.com/e77cea9eaadf33e1d26d38d25e52ecb0be9bbb24/base/allocator/partition_allocator/page_allocator.cc