- Add and enable concurrent sweeper.

- Add and enable concurrent sweeper.
- Keep separate lists for normal sized executable and data pages.

R=koda@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39593

[Ivan Posva, 2014-08-26 21:05:44]

iposva@google.com changed reviewers:
+ asiva@google.com, koda@google.com

[koda, 2014-08-26 23:34:49]

First pass

https://codereview.chromium.org/503363005/diff/1/runtime/vm/class_table.h
File runtime/vm/class_table.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/class_table.h#new...
runtime/vm/class_table.h:116: explicit ClassTable(ClassTable* original);
const ClassTable*

And comment that stats are not copied.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/gc_sweeper.cc
File runtime/vm/gc_sweeper.cc (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/gc_sweeper.cc#new...
runtime/vm/gc_sweeper.cc:130: prev_page = page;
You are silently skipping non-kData pages. This should at the least be clearly
documented, or better, just make it a precondition that only kData-pages are
passed to SweepConcurrent (now that we have segregated the lists), and ASSERT
here instead.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/gc_sweeper.h
File runtime/vm/gc_sweeper.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/gc_sweeper.h#newc...
runtime/vm/gc_sweeper.h:35: static void SweepConcurrent(Isolate* isolate,
Comment on:
* whether 'last' is exclusive
* locking state of 'freelist'
* can 'isolate' be anything else than the owner of 'old_space'?

https://codereview.chromium.org/503363005/diff/1/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/heap.cc#newcode88
runtime/vm/heap.cc:88: while ((addr == 0) && (old_space_->tasks())) {
Add missing "> 0" (or, per my other comment, "has_tasks()")

https://codereview.chromium.org/503363005/diff/1/runtime/vm/heap.cc#newcode95
runtime/vm/heap.cc:95: addr = old_space_->TryAllocate(size, type,
PageSpace::kForceGrowth);
Explain why there's no point in waiting for sweepers here.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:449: : store_buffer_(true),
add /* shallow_copy = */ to make this slightly less unreadable...

https://codereview.chromium.org/503363005/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/isolate.h#newcode123
runtime/vm/isolate.h:123: Isolate* ShallowCopy();
Briefly specify what is available in a shallow copy:
* class table (without stats)
* ...
and preferably also why it's useful.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.cc#newcode126
runtime/vm/pages.cc:126: pages_lock_(new Mutex),
Mutex -> Mutex()

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.cc#newcode183
runtime/vm/pages.cc:183: const bool is_protected = is_exec &&
FLAG_write_protect_code;
"is_protected" relates to exec_pages_tail_, which is always executable. So it
doesn't make sense to use "is_exec", which relates to the page you are
allocating, in this expression. Just remove "is_exec" here.

That said, I really think it's about time to create a PageList utility class to
avoid lots of duplication and risk for bugs like this one.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.h
File runtime/vm/pages.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.h#newcode301
runtime/vm/pages.h:301: void set_tasks(intptr_t val) { tasks_ = val; }
ASSERT(tasks_ >= 0);

Also consider just having increment/decrement/has_tasks methods.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.h#newcode356
runtime/vm/pages.h:356: HeapPage* large_pages_;
Can large pages be executable?

https://codereview.chromium.org/503363005/diff/1/runtime/vm/store_buffer.h
File runtime/vm/store_buffer.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/store_buffer.h#ne...
runtime/vm/store_buffer.h:56: // The value shallow_copy is only used to select
this non-allocating
Consider creating a useful parameter instead, like num_initial_blocks.

[Ivan Posva, 2014-08-27 01:00:22]

https://codereview.chromium.org/503363005/diff/1/runtime/vm/class_table.h
File runtime/vm/class_table.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/class_table.h#new...
runtime/vm/class_table.h:116: explicit ClassTable(ClassTable* original);
On 2014/08/26 23:34:48, koda wrote:
> const ClassTable*
> 
> And comment that stats are not copied.

Done.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/gc_sweeper.cc
File runtime/vm/gc_sweeper.cc (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/gc_sweeper.cc#new...
runtime/vm/gc_sweeper.cc:130: prev_page = page;
On 2014/08/26 23:34:48, koda wrote:
> You are silently skipping non-kData pages. This should at the least be clearly
> documented, or better, just make it a precondition that only kData-pages are
> passed to SweepConcurrent (now that we have segregated the lists), and ASSERT
> here instead.

Changed to an assertion that only data pages are processed.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/gc_sweeper.h
File runtime/vm/gc_sweeper.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/gc_sweeper.h#newc...
runtime/vm/gc_sweeper.h:35: static void SweepConcurrent(Isolate* isolate,
On 2014/08/26 23:34:48, koda wrote:
> Comment on:
> * whether 'last' is exclusive
> * locking state of 'freelist'
> * can 'isolate' be anything else than the owner of 'old_space'?

Done.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/heap.cc#newcode88
runtime/vm/heap.cc:88: while ((addr == 0) && (old_space_->tasks())) {
On 2014/08/26 23:34:48, koda wrote:
> Add missing "> 0" (or, per my other comment, "has_tasks()")

Uiuiuiui! That should not have happened.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/heap.cc#newcode95
runtime/vm/heap.cc:95: addr = old_space_->TryAllocate(size, type,
PageSpace::kForceGrowth);
On 2014/08/26 23:34:48, koda wrote:
> Explain why there's no point in waiting for sweepers here.

We should be waiting for sweepers here, but I missed it during refactoring of
this code. Thanks!

https://codereview.chromium.org/503363005/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:449: : store_buffer_(true),
On 2014/08/26 23:34:48, koda wrote:
> add /* shallow_copy = */ to make this slightly less unreadable...

ditto.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/isolate.h#newcode123
runtime/vm/isolate.h:123: Isolate* ShallowCopy();
On 2014/08/26 23:34:48, koda wrote:
> Briefly specify what is available in a shallow copy:
> * class table (without stats)
> * ...
> and preferably also why it's useful.

For now we are leaving this alone after discussion. Some major refactoring will
be needed for the use of isolate/classtable.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.cc#newcode126
runtime/vm/pages.cc:126: pages_lock_(new Mutex),
On 2014/08/26 23:34:48, koda wrote:
> Mutex -> Mutex()

Done.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.cc#newcode183
runtime/vm/pages.cc:183: const bool is_protected = is_exec &&
FLAG_write_protect_code;
On 2014/08/26 23:34:49, koda wrote:
> "is_protected" relates to exec_pages_tail_, which is always executable. So it
> doesn't make sense to use "is_exec", which relates to the page you are
> allocating, in this expression. Just remove "is_exec" here.
> 
> That said, I really think it's about time to create a PageList utility class
to
> avoid lots of duplication and risk for bugs like this one.

Done.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.h
File runtime/vm/pages.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.h#newcode301
runtime/vm/pages.h:301: void set_tasks(intptr_t val) { tasks_ = val; }
On 2014/08/26 23:34:49, koda wrote:
> ASSERT(tasks_ >= 0);
> 
> Also consider just having increment/decrement/has_tasks methods.

Done.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/pages.h#newcode356
runtime/vm/pages.h:356: HeapPage* large_pages_;
On 2014/08/26 23:34:49, koda wrote:
> Can large pages be executable?

Yes, large pages survive as a mixed class list.

https://codereview.chromium.org/503363005/diff/1/runtime/vm/store_buffer.h
File runtime/vm/store_buffer.h (right):

https://codereview.chromium.org/503363005/diff/1/runtime/vm/store_buffer.h#ne...
runtime/vm/store_buffer.h:56: // The value shallow_copy is only used to select
this non-allocating
On 2014/08/26 23:34:49, koda wrote:
> Consider creating a useful parameter instead, like num_initial_blocks.

Leaving this one alone as well. The need for this will go away if we can remove
the shallow proxy copy of isolates.

[koda, 2014-08-27 05:42:20]

lgtm if you fix the last thing

https://codereview.chromium.org/503363005/diff/20001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/503363005/diff/20001/runtime/vm/heap.cc#newco...
runtime/vm/heap.cc:112: addr = old_space_->TryAllocate(size, type);
Shouldn't this have kForceGrowth?

[Ivan Posva, 2014-08-27 13:38:04]

Committed patchset #2 manually as r39593 (presubmit successful).