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

runtime/vm/scavenger.h

Index: runtime/vm/scavenger.h
diff --git a/runtime/vm/scavenger.h b/runtime/vm/scavenger.h
index 8d8c0e49e3fa121840f8d51ba958548d3c5401b9..a50f072e9d6241440770e70271e366c3b9094009 100644
--- a/runtime/vm/scavenger.h
+++ b/runtime/vm/scavenger.h
@@ -126,21 +126,49 @@ class Scavenger {
   uword TryAllocate(intptr_t size) {
     ASSERT(Utils::IsAligned(size, kObjectAlignment));
     ASSERT(heap_ != Dart::vm_isolate()->heap());
+
 #if defined(DEBUG)
     if (FLAG_gc_at_alloc && !scavenging_) {
       Scavenge();
     }
 #endif
-    uword result = top_;
-    intptr_t remaining = end_ - top_;
-    if (remaining < size) {
+
+    Thread* thread = Thread::Current();

[rmacnak, 2017/07/05 17:39:53]

This will hurt DBC as TLS is expensive. Pass in the Thread instead. 

[danunez, 2017/07/05 18:12:55]

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

+    uword top = 0;
+    uword end = 0;
+
+    if (!thread->IsMutatorThread()) {
+      thread = Isolate::Current()->mutator_thread();

[rmacnak, 2017/07/05 17:39:53]

We cannot access state on another Thread outside of a SafepointOperationScope.

ASSERT(thread->IsMutator());

[danunez, 2017/07/05 18:12:56]

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.

+    }
+
+    if (thread->heap() != NULL) {

[rmacnak, 2017/07/05 17:39:53]

This shouldn't be possible unless we haven't correctly initialized the thread.

[danunez, 2017/07/05 18:12:56]

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

+      top = thread->top();
+      end = thread->end();
+    } else {
+      top = top_;
+      end = end_;
+    }
+
+    uword result = top;
+    intptr_t remaining = end - top;
+    if ((remaining < size) || (CapacityInWords() == 0)) {

[rmacnak, 2017/07/05 17:39:53]

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.

[danunez, 2017/07/05 18:12:56]

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.

       return 0;
     }
     ASSERT(to_->Contains(result));
     ASSERT((result & kObjectAlignmentMask) == object_alignment_);
 
-    top_ += size;
-    ASSERT(to_->Contains(top_) || (top_ == to_->end()));
+    top += size;
+    ASSERT(to_->Contains(top) || (top == to_->end()));
+
+    if (thread->heap() != NULL) {
+      thread->set_top_offset(top);
+    } else {
+      top_ = top;
+    }
+    // We only want to change top_ if mutator is scheduled and therefore
+    // has a heap attached.
+
+
     return result;
   }
 
@@ -158,8 +186,6 @@ class Scavenger {
   // Accessors to generate code for inlined allocation.
   uword* TopAddress() { return &top_; }
   uword* EndAddress() { return &end_; }
-  static intptr_t top_offset() { return OFFSET_OF(Scavenger, top_); }
-  static intptr_t end_offset() { return OFFSET_OF(Scavenger, end_); }
 
   int64_t UsedInWords() const {
     return (top_ - FirstObjectStart()) >> kWordSizeLog2;
@@ -235,20 +261,75 @@ class Scavenger {
   // not consume space in the to space they leave enough room for this stack.
   void PushToPromotedStack(uword addr) {

[rmacnak, 2017/07/05 17:39:53]

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.

[danunez, 2017/07/05 18:12:56]

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.

     ASSERT(scavenging_);
-    end_ -= sizeof(addr);
-    ASSERT(end_ > top_);
-    *reinterpret_cast<uword*>(end_) = addr;
+
+    uword top = 0;
+    uword end = 0;
+    Thread* thread = Thread::Current();
+    if (!thread->IsMutatorThread()) {
+      thread = Isolate::Current()->mutator_thread();
+    }
+
+    if (thread->heap() != NULL) {
+      top = thread->top();
+      end = thread->end();
+    } else {
+      top = top_;
+      end = end_;
+    }
+
+    end -= sizeof(addr);
+
+    if (thread->heap() != NULL) {
+      thread->set_end_offset(end);
+    } else {
+      end_ = end;
+    }
+
+    ASSERT(end > top);
+    *reinterpret_cast<uword*>(end) = addr;
   }
   uword PopFromPromotedStack() {
     ASSERT(scavenging_);
-    uword result = *reinterpret_cast<uword*>(end_);
-    end_ += sizeof(result);
-    ASSERT(end_ <= to_->end());
+
+    uword end = 0;
+    Thread* thread = Thread::Current();
+    if (!thread->IsMutatorThread()) {
+      thread = Isolate::Current()->mutator_thread();
+    }
+
+    if (thread->heap() != NULL) {
+      end = thread->end();
+    } else {
+      end = end_;
+    }
+
+    uword result = *reinterpret_cast<uword*>(end);
+    end += sizeof(result);
+
+    if (thread->heap() != NULL) {
+      thread->set_end_offset(end);
+    } else {
+      end_ = end;
+    }
+
+    ASSERT(end <= to_->end());
     return result;
   }
   bool PromotedStackHasMore() const {
     ASSERT(scavenging_);
-    return end_ < to_->end();
+    uword end = 0;
+    Thread* thread = Thread::Current();
+    if (!thread->IsMutatorThread()) {
+      thread = Isolate::Current()->mutator_thread();
+    }
+
+    if (thread->heap() != NULL) {
+      end = thread->end();
+    } else {
+      end = end_;
+    }
+
+    return end < to_->end();
   }
 
   void UpdateMaxHeapCapacity();
@@ -297,6 +378,8 @@ class Scavenger {
   // The total size of external data associated with objects in this scavenger.
   intptr_t external_size_;
 
+  Mutex* space_lock_;

[rmacnak, 2017/07/05 17:39:53]

Add a comment describing what this lock protects.

[danunez, 2017/07/05 18:12:56]

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

+
   friend class ScavengerVisitor;
   friend class ScavengerWeakVisitor;