Moves the top_ and end_ words of the Scavenger into mutator thread.

Moves the top_ and end_ words of the Scavenger into mutator thread.

This is the first step to adding Thread Local Allocation Buffers to
the VM.

In this step, the mutator alone allocates to the new space, but keeps
track of the start and end of the space. This is akin to a single large
TLAB.
BUG=
R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/710544a9078e013aba49f0f00fc735526ac896bc

[danunez, 2017-06-22 21:44:26]

Description was changed from

==========
Moves the top_ and end_ words of the Scavenger into mutator thread.

This is the first step to adding Thread Local Allocation Buffers to
the VM.

In this step, the mutator alone allocates to the new space, but keeps
track of the start and end of the space. This is akin to a single large
TLAB.
BUG=
==========

to

==========
Moves the top_ and end_ words of the Scavenger into mutator thread.

This is the first step to adding Thread Local Allocation Buffers to
the VM.

In this step, the mutator alone allocates to the new space, but keeps
track of the start and end of the space. This is akin to a single large
TLAB.
BUG=
==========

[danunez, 2017-06-22 21:44:27]

danunez@google.com changed reviewers:
+ rmacnak@google.com

[rmacnak, 2017-06-22 22:31:51]

Try running with --no-inline-alloc to rule out a problem with the generated
code.

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/heap.h#newcode132
runtime/vm/heap.h:132: static intptr_t TopOffset(Space space);
Both should be unused, no?

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2661: thread->top_ = *(heap()->new_space()->TopAddress());
Since we don't yet have background new-space allocation, only the mutator thread
should copy top from the semispace, and the helper threads should have 0s so
allocation fails.

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:742: while (cur < top_) {
top_ may be stale

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:751: while (cur < top_) {
top_ may be stale

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:767: if (visitor->VisitRange(cur, top_)) {
top_ may be stale

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.h#newc...
runtime/vm/scavenger.h:131: ASSERT(Isolate::Current()->mutator_thread()->top()
== top_);
Thread::Current() should be the mutator

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.h#newc...
runtime/vm/scavenger.h:163: // Accessors to generate code for inlined
allocation.
Remove to check these are all unused now.

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/thread.h#newcode770
runtime/vm/thread.h:770: uword top_;
initialize to 0 in Thread::Thread

[danunez, 2017-06-30 20:35:58]

Expect a new patch soon for review. Will be on the lookout for more
non-deterministic bugs.

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/heap.h#newcode132
runtime/vm/heap.h:132: static intptr_t TopOffset(Space space);
On 2017/06/22 22:31:50, rmacnak wrote:
> Both should be unused, no?

Yes, but I left this in for testing. Incoming set will remove them.

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2661: thread->top_ = *(heap()->new_space()->TopAddress());
On 2017/06/22 22:31:50, rmacnak wrote:
> Since we don't yet have background new-space allocation, only the mutator
thread
> should copy top from the semispace, and the helper threads should have 0s so
> allocation fails.

Done.

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:742: while (cur < top_) {
On 2017/06/22 22:31:50, rmacnak wrote:
> top_ may be stale

Right. top_ is now retrieved from the mutator thread if it is still scheduled.

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:751: while (cur < top_) {
On 2017/06/22 22:31:50, rmacnak wrote:
> top_ may be stale

Right. top_ is now retrieved from the mutator thread if it is still scheduled.

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:767: if (visitor->VisitRange(cur, top_)) {
On 2017/06/22 22:31:50, rmacnak wrote:
> top_ may be stale

Right. top_ is now retrieved from the mutator thread if it is still scheduled.

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.h#newc...
runtime/vm/scavenger.h:131: ASSERT(Isolate::Current()->mutator_thread()->top()
== top_);
On 2017/06/22 22:31:51, rmacnak wrote:
> Thread::Current() should be the mutator

Done.

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/scavenger.h#newc...
runtime/vm/scavenger.h:163: // Accessors to generate code for inlined
allocation.
On 2017/06/22 22:31:50, rmacnak wrote:
> Remove to check these are all unused now.

TopAddress() and EndAddress() are used, but only to initialize the mutator's
top_ and end_.

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

https://codereview.chromium.org/2951333002/diff/1/runtime/vm/thread.h#newcode770
runtime/vm/thread.h:770: uword top_;
On 2017/06/22 22:31:51, rmacnak wrote:
> initialize to 0 in Thread::Thread

Done.

[rmacnak, 2017-07-05 17:39:54]

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

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:131: // Accessors for inlined allocation in generated code.
Remove comment too.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:333: ALIGN8 Scavenger new_space_;
This should no longer be accessed from generated code. Remove ALIGN8.

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

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2670: mutator_thread_ = thread;
// Grab an allocation area.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2671: if (this != Dart::vm_isolate()) {
Is there a property of the heap we can check instead? Perhaps a zero-sized
new-space?

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2672: mutator_thread_->top_ =
*(heap()->new_space()->TopAddress());
*top_address() -> top()

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2714: if (this != Dart::vm_isolate()) {
// Release allocation area.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2715: *(heap()->new_space()->TopAddress()) =
mutator_thread_->top_;
*top_address() -> set_top()

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

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:141: new_addr = scavenger_->TryAllocate(size);
Can we use a separate allocation function here? During a scavenge, we should be
able to use Scavenger's top/end instead of Thread's. We should also be able to
assert copying allocations during a scavenge always succeed.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:160: new_addr = scavenger_->TryAllocate(size);
And here.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:398: Thread* thread = Thread::Current();
How about

Thread* mutator = isolate->mutator_thread()

?

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:803: if (!thread->IsMutatorThread()) {
Ditto

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:824: if (!thread->IsMutatorThread()) {
Ditto

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:851: if (!thread->IsMutatorThread()) {
Ditto

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h
File runtime/vm/scavenger.h (right):

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:136: Thread* thread = Thread::Current();
This will hurt DBC as TLS is expensive. Pass in the Thread instead.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:141: thread = Isolate::Current()->mutator_thread();
We cannot access state on another Thread outside of a SafepointOperationScope.

ASSERT(thread->IsMutator());

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:144: if (thread->heap() != NULL) {
This shouldn't be possible unless we haven't correctly initialized the thread.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:154: if ((remaining < size) || (CapacityInWords() == 0))
{
Why is the second check needed? Only the VM isolate should have an empty new
space, and it should never do a new-space allocation as asserted in L128.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:262: void PushToPromotedStack(uword addr) {
Promo stack:

During a scavenge, we should be using Scavenger's top/end instead of Thread's
top/end. So all this promo stack code should not need to change.

More generally, we should not need to access TLS in the middle of scavenging.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:381: Mutex* space_lock_;
Add a comment describing what this lock protects.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:367: static intptr_t heap_offset() { return
OFFSET_OF(Thread, heap_); }
It should be possible to remove this now, right?

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:369: void set_top_offset(uword value) {
set_top

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:373: void set_end_offset(uword value) {
set_end

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:770: uword top_;
These should move up to where heap_ is. See comment about fields accessed from
generated code.

[danunez, 2017-07-05 18:12:57]

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

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:131: // Accessors for inlined allocation in generated code.
On 2017/07/05 17:39:52, rmacnak wrote:
> Remove comment too.

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:333: ALIGN8 Scavenger new_space_;
On 2017/07/05 17:39:52, rmacnak wrote:
> This should no longer be accessed from generated code. Remove ALIGN8.

I will try this and see what happens. I think you are right though.

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

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2670: mutator_thread_ = thread;
On 2017/07/05 17:39:52, rmacnak wrote:
> // Grab an allocation area.

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2671: if (this != Dart::vm_isolate()) {
On 2017/07/05 17:39:52, rmacnak wrote:
> Is there a property of the heap we can check instead? Perhaps a zero-sized
> new-space?

That should work. I'll give it a shot.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2672: mutator_thread_->top_ =
*(heap()->new_space()->TopAddress());
On 2017/07/05 17:39:52, rmacnak wrote:
> *top_address() -> top()

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2714: if (this != Dart::vm_isolate()) {
On 2017/07/05 17:39:52, rmacnak wrote:
> // Release allocation area.

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2715: *(heap()->new_space()->TopAddress()) =
mutator_thread_->top_;
On 2017/07/05 17:39:52, rmacnak wrote:
> *top_address() -> set_top()

Done.

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

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:141: new_addr = scavenger_->TryAllocate(size);
On 2017/07/05 17:39:52, rmacnak wrote:
> Can we use a separate allocation function here? During a scavenge, we should
be
> able to use Scavenger's top/end instead of Thread's. We should also be able to
> assert copying allocations during a scavenge always succeed.

I will add a function to do allocation using the thread's top and end. This
function will become the TLAB allocation function. TryAllocate(intptr_t) will be
reverted to the version in master.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:160: new_addr = scavenger_->TryAllocate(size);
On 2017/07/05 17:39:52, rmacnak wrote:
> And here.

Ditto.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:398: Thread* thread = Thread::Current();
On 2017/07/05 17:39:53, rmacnak wrote:
> How about
> 
> Thread* mutator = isolate->mutator_thread()
> 
> ?

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:803: if (!thread->IsMutatorThread()) {
On 2017/07/05 17:39:53, rmacnak wrote:
> Ditto

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:824: if (!thread->IsMutatorThread()) {
On 2017/07/05 17:39:53, rmacnak wrote:
> Ditto

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:851: if (!thread->IsMutatorThread()) {
On 2017/07/05 17:39:52, rmacnak wrote:
> Ditto

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h
File runtime/vm/scavenger.h (right):

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:136: Thread* thread = Thread::Current();
On 2017/07/05 17:39:53, rmacnak wrote:
> This will hurt DBC as TLS is expensive. Pass in the Thread instead. 

I will do this as part of a separate function, as you suggested in a later
comment in scavenger.cc.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:141: thread = Isolate::Current()->mutator_thread();
On 2017/07/05 17:39:53, rmacnak wrote:
> We cannot access state on another Thread outside of a SafepointOperationScope.
> 
> ASSERT(thread->IsMutator());

Can't the background thread trigger a full heap GC? This would mean a
non-mutator thread is doing new space GC, which is what I found during testing.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:144: if (thread->heap() != NULL) {
On 2017/07/05 17:39:53, rmacnak wrote:
> This shouldn't be possible unless we haven't correctly initialized the thread.

This happens if the mutator is unscheduled, but the background thread triggers a
GC. This was how I checked if the mutator was unscheduled.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:154: if ((remaining < size) || (CapacityInWords() == 0))
{
On 2017/07/05 17:39:53, rmacnak wrote:
> Why is the second check needed? Only the VM isolate should have an empty new
> space, and it should never do a new-space allocation as asserted in L128.

Correct. There is exactly one test that checks the semantics of a zero size
Scavenger in C++. This check is only for that test. I could not think of any
other way to deal with it.

I am open to suggestions.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:262: void PushToPromotedStack(uword addr) {
On 2017/07/05 17:39:53, rmacnak wrote:
> Promo stack:
> 
> During a scavenge, we should be using Scavenger's top/end instead of Thread's
> top/end. So all this promo stack code should not need to change.
> 
> More generally, we should not need to access TLS in the middle of scavenging.

I do not recall why I changed this to use TLS. I will change it back and use the
Epilogue to set the new top and end on the stack. By then, nothing should be
touching the heap.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:381: Mutex* space_lock_;
On 2017/07/05 17:39:53, rmacnak wrote:
> Add a comment describing what this lock protects.

I will remove the lock for now. It has no purpose in this CL.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:367: static intptr_t heap_offset() { return
OFFSET_OF(Thread, heap_); }
On 2017/07/05 17:39:53, rmacnak wrote:
> It should be possible to remove this now, right?

Looks like it, according to a grep. Consider it gone.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:369: void set_top_offset(uword value) {
On 2017/07/05 17:39:53, rmacnak wrote:
> set_top

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:373: void set_end_offset(uword value) {
On 2017/07/05 17:39:53, rmacnak wrote:
> set_end

Done.

https://codereview.chromium.org/2951333002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:770: uword top_;
On 2017/07/05 17:39:53, rmacnak wrote:
> These should move up to where heap_ is. See comment about fields accessed from
> generated code.

I placed these down here after I encountered an error during testing about bad
offsets. I will try again and see if I encounter it.

[rmacnak, 2017-07-12 17:10:22]

I think we're almost there.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:400: Thread* thread = Thread::Current();
Why not always use `isolate->mutator_thread()`?

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:423: if (thread->heap() == heap_) {
We should never scavenge another isolate's heap. (Though we may iterate another
isolate's heap.)

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:444: if (thread->heap() == heap_) {
We should never scavenge another isolate's heap. (Though we may iterate another
isolate's heap.)

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:585: 
Spurious whitespace changes.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:954: void Scavenger::Evacuate(Thread* thread) {
It looks like all callers pass isolate()->mutator_thread(), so why not just use
that here?

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:966: if (thread->heap() != NULL) {
// Null in some unit tests.     (Presumably)

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.h
File runtime/vm/scavenger.h (right):

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:191: // Accessors to generate code for inlined
allocation.
Remove TopAddress and EndAddress.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:280: 
Remove spurious whitespace changes.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:307: // Accessed from generated code.
Remove comment. No longer accessed from generated code.

And comment about the true value residing on the mutator Thread object during
regular program execution, and residing here during a GC/iteration.

[danunez, 2017-07-12 18:23:38]

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:400: Thread* thread = Thread::Current();
On 2017/07/12 17:10:22, rmacnak wrote:
> Why not always use `isolate->mutator_thread()`?

Looks like I missed one.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:423: if (thread->heap() == heap_) {
On 2017/07/12 17:10:22, rmacnak wrote:
> We should never scavenge another isolate's heap. (Though we may iterate
another
> isolate's heap.)

As discussed, will replace with a check if the mutator is scheduled. Originally
kept this for the VM isolate, but as you stated, VM isolate should never be
scavenging.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:444: if (thread->heap() == heap_) {
On 2017/07/12 17:10:22, rmacnak wrote:
> We should never scavenge another isolate's heap. (Though we may iterate
another
> isolate's heap.)

Done.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:585: 
On 2017/07/12 17:10:22, rmacnak wrote:
> Spurious whitespace changes.

Done.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:954: void Scavenger::Evacuate(Thread* thread) {
On 2017/07/12 17:10:22, rmacnak wrote:
> It looks like all callers pass isolate()->mutator_thread(), so why not just
use
> that here?

You are right. I will modify this function to grab the mutator thread.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:966: if (thread->heap() != NULL) {
On 2017/07/12 17:10:22, rmacnak wrote:
> // Null in some unit tests.     (Presumably)

As stated above, will replace with a function that checks if the mutator is
actually scheduled.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.h
File runtime/vm/scavenger.h (right):

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:191: // Accessors to generate code for inlined
allocation.
On 2017/07/12 17:10:22, rmacnak wrote:
> Remove TopAddress and EndAddress.

Done.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:280: 
On 2017/07/12 17:10:22, rmacnak wrote:
> Remove spurious whitespace changes. 

Done.

https://codereview.chromium.org/2951333002/diff/40001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:307: // Accessed from generated code.
On 2017/07/12 17:10:22, rmacnak wrote:
> Remove comment. No longer accessed from generated code.
> 
> And comment about the true value residing on the mutator Thread object during
> regular program execution, and residing here during a GC/iteration.

Done.

[danunez, 2017-07-12 20:13:53]

New patchset.

[rmacnak, 2017-07-12 20:30:07]

lgtm w/ comments

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

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:66: uword addr =
new_space_.TryAllocateInTLAB(Thread::Current(), size);
Hoist Thread::Current() so it is only called once.

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h
File runtime/vm/scavenger.h (right):

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:126: uword TryAllocate(intptr_t size) { return
TryAllocateGC(size); }
Dead code, remove.

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:128: uword TryAllocateGC(intptr_t size) {
No "Try". Allocation during a scavenge always succeeds.

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:133: if (FLAG_gc_at_alloc && !scavenging_) {
Remove, this is always called during a GC.

ASSERT(scavenging_)

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:139: if (remaining < size) {
ASSERT(size <= remaining);

[danunez, 2017-07-12 21:02:12]

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

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:66: uword addr =
new_space_.TryAllocateInTLAB(Thread::Current(), size);
On 2017/07/12 20:30:06, rmacnak wrote:
> Hoist Thread::Current() so it is only called once.

Done.

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h
File runtime/vm/scavenger.h (right):

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:126: uword TryAllocate(intptr_t size) { return
TryAllocateGC(size); }
On 2017/07/12 20:30:06, rmacnak wrote:
> Dead code, remove.

Done.

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:128: uword TryAllocateGC(intptr_t size) {
On 2017/07/12 20:30:06, rmacnak wrote:
> No "Try". Allocation during a scavenge always succeeds.

Done.

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:133: if (FLAG_gc_at_alloc && !scavenging_) {
On 2017/07/12 20:30:06, rmacnak wrote:
> Remove, this is always called during a GC.
> 
> ASSERT(scavenging_)

Done.

https://codereview.chromium.org/2951333002/diff/60001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:139: if (remaining < size) {
On 2017/07/12 20:30:06, rmacnak wrote:
> ASSERT(size <= remaining);

Done.

[danunez, 2017-07-12 21:09:11]

Description was changed from

==========
Moves the top_ and end_ words of the Scavenger into mutator thread.

This is the first step to adding Thread Local Allocation Buffers to
the VM.

In this step, the mutator alone allocates to the new space, but keeps
track of the start and end of the space. This is akin to a single large
TLAB.
BUG=
==========

to

==========
Moves the top_ and end_ words of the Scavenger into mutator thread.

This is the first step to adding Thread Local Allocation Buffers to
the VM.

In this step, the mutator alone allocates to the new space, but keeps
track of the start and end of the space. This is akin to a single large
TLAB.
BUG=
R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/710544a9078e013aba49f0f00fc735526ac896bc
==========

[danunez, 2017-07-12 21:09:12]

Committed patchset #5 (id:80001) manually as
710544a9078e013aba49f0f00fc735526ac896bc (presubmit successful).