Re-land "Concurrently unmap free pages."

Re-land "Concurrently unmap free pages."

BUG=chromium:507211
LOG=n

Committed: https://crrev.com/f0e3d51840634b2648391e85469e6f2ef6247dae
Cr-Commit-Position: refs/heads/master@{#30335}

[Hannes Payer (out of office), 2015-08-24 13:00:41]

hpayer@chromium.org changed reviewers:
+ mlippautz@chromium.org

[Michael Lippautz, 2015-08-24 13:43:12]

Mostly nits and food for thought.

https://codereview.chromium.org/1306183003/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1306183003/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:6583: void Heap::FreeQueuedChunks(MemoryChunk* list_head) {
This is a candidate for a static on {MemoryChunk}?

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:80: code_range_(NULL),
add
  free_list_mutex_(),

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:166: base::LockGuard<base::Mutex>
free_list_lock_guard(&free_list_mutex_);
Please add a block to make clear which parts are to-be-locked.

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:345: LOG(isolate_, DeleteEvent("InitialChunk", addr));
"InitialChunk"?

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:348: size_t size = reservation->size();
const size_t size = ..

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:747: void MemoryAllocator::PreFreeMemory(MemoryChunk* chunk)
{
Should we add a flag to {MemoryChunk} indicating that it has been PreFreed()?
(The flag could be checked in PerformFree as DCHECK.)

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:776: void MemoryAllocator::PerformFreeMemory(MemoryChunk*
chunk) {
We could make this a static on {MemoryChunk}. 

{MemoryChunk} is sort of statically created (just cast to a MemoryChunk after
allocating it, so it would probably make sense to destroy it the same way.)

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.h#newco...
src/heap/spaces.h:1088: // bookkeeping and calls the allocation callback.
Can we extend this comment to clarify that this code is meant to be run on the
main thread?

[Hannes Payer (out of office), 2015-08-24 14:58:13]

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:80: code_range_(NULL),
On 2015/08/24 13:43:12, Michael Lippautz wrote:
> add
>   free_list_mutex_(),

Why? The default constructor is called in this case anyway.

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:166: base::LockGuard<base::Mutex>
free_list_lock_guard(&free_list_mutex_);
On 2015/08/24 13:43:12, Michael Lippautz wrote:
> Please add a block to make clear which parts are to-be-locked.

I would have just locked the rest of the function, but we can do better. Here is
your scope. Done.

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:345: LOG(isolate_, DeleteEvent("InitialChunk", addr));
On 2015/08/24 13:43:12, Michael Lippautz wrote:
> "InitialChunk"?

This was the original string. We can change it to NewSpace.

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:348: size_t size = reservation->size();
On 2015/08/24 13:43:12, Michael Lippautz wrote:
> const size_t size = ..

Done.

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:747: void MemoryAllocator::PreFreeMemory(MemoryChunk* chunk)
{
On 2015/08/24 13:43:12, Michael Lippautz wrote:
> Should we add a flag to {MemoryChunk} indicating that it has been PreFreed()?
> (The flag could be checked in PerformFree as DCHECK.)

Good idea. Done.

https://codereview.chromium.org/1306183003/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:776: void MemoryAllocator::PerformFreeMemory(MemoryChunk*
chunk) {
On 2015/08/24 13:43:12, Michael Lippautz wrote:
> We could make this a static on {MemoryChunk}. 
> 
> {MemoryChunk} is sort of statically created (just cast to a MemoryChunk after
> allocating it, so it would probably make sense to destroy it the same way.)

As discussed offline, this method should not be static since it calls deeper
into the memory allocator. However, it makes sense to perform the deallocation
of the previously alloced data structures in a method on the memory chunk.

[Michael Lippautz, 2015-08-24 16:06:25]

lgtm % build

[Hannes Payer (out of office), 2015-08-24 16:06:55]

The CQ bit was checked by hpayer@chromium.org

[commit-bot: I haz the power, 2015-08-24 16:07:07]

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

[commit-bot: I haz the power, 2015-08-24 16:27:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-24 16:28:02]

Patchset 6 (id:??) landed as
https://crrev.com/f0e3d51840634b2648391e85469e6f2ef6247dae
Cr-Commit-Position: refs/heads/master@{#30335}