Prepares allocation for proper sync with mutator and bg threads.

Prepares allocation for proper sync with mutator and bg threads.

When top_ is required, we now search through all active threads and pull
the top_ furthest in the space.

When searching the space, we fills every thread's TLAB with a
FreeListElement. Note that future allocations can safely overwrite
them. We do NOT abandon the TLAB (i.e. set top_,end_ to 0) in these
cases.

Added notes to determing where to lock/unlock for threads

R=asiva@google.com, rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/78745901799347a9184689b01c92a039fbd76698

[rmacnak, 2017-08-01 21:51:25]

I like the trick about allowing further allocation on top of the
FreeListElement.

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

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/heap.cc#newcode90
runtime/vm/heap.cc:90: ASSERT(Thread::Current()->IsMutatorThread());
Isn't this the same as the line above?

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

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/heap.h#newcode260
runtime/vm/heap.h:260: void FillRemainingTLAB(Thread* thread);
"MakeTLABIterable" ?   (Compare PageSpace::MakeIterable)

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

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:722: uword furthest_addr = 0;
If we make all outstanding TLABs walkable, everything from Scavenger::base_ to
Scavenger::top_ should be walkable, right? Then there is no need to find the
highest top among the TLABs or to copy from a TLAB to the Scavenger and back.

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:734: ASSERT(heap_ != NULL);
Per comment above, I think this could be replaced with MakeAllTLABsIterable...

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:747: void Scavenger::UnflushTLS(uword value) const {
...which should make this a no-op.

[danunez, 2017-08-01 23:25:35]

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

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/heap.cc#newcode90
runtime/vm/heap.cc:90: ASSERT(Thread::Current()->IsMutatorThread());
On 2017/08/01 21:51:25, rmacnak wrote:
> Isn't this the same as the line above?

Yes it is. I will remove it.

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

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/heap.h#newcode260
runtime/vm/heap.h:260: void FillRemainingTLAB(Thread* thread);
On 2017/08/01 21:51:25, rmacnak wrote:
> "MakeTLABIterable" ?   (Compare PageSpace::MakeIterable)

Done.

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

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:722: uword furthest_addr = 0;
On 2017/08/01 21:51:25, rmacnak wrote:
> If we make all outstanding TLABs walkable, everything from Scavenger::base_ to
> Scavenger::top_ should be walkable, right? Then there is no need to find the
> highest top among the TLABs or to copy from a TLAB to the Scavenger and back.

That make sense. However, I am going to split this into two functions.
1) MakeALLTLABsIterable: Fills all TLABS to make the new space iterable.
2) FindTopOfSpace: Returns the address of the last object in the new space.

2) is specifically for UsedInWords() so we do not have an incorrect size
reported in Observatory (https://github.com/dart-lang/sdk/issues/30311).

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:734: ASSERT(heap_ != NULL);
On 2017/08/01 21:51:25, rmacnak wrote:
> Per comment above, I think this could be replaced with MakeAllTLABsIterable...

Done.

https://codereview.chromium.org/2992753002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:747: void Scavenger::UnflushTLS(uword value) const {
On 2017/08/01 21:51:25, rmacnak wrote:
> ...which should make this a no-op.

Done.

[rmacnak, 2017-08-02 19:20:48]

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

https://codereview.chromium.org/2992753002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:817: // TODO(danunez): Abandon all threads' TLABs. Every
thread should be stopped
Why TODO? Looks implemented to me.

https://codereview.chromium.org/2992753002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:963: uword top_of_space =
FindTopOfSpace(heap_->isolate());
This will count of the unallocated space in all but the highest TLAB as "used".
What about starting with Scavenger::top_ - Scavenger::object_start() and
subtracting out the unallocated space from each TLAB?

[danunez, 2017-08-02 19:36:08]

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

https://codereview.chromium.org/2992753002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:817: // TODO(danunez): Abandon all threads' TLABs. Every
thread should be stopped
On 2017/08/02 19:20:48, rmacnak wrote:
> Why TODO? Looks implemented to me.

It is implemented (minus some locking with thread_lock()). This TODO will be
removed.

https://codereview.chromium.org/2992753002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:963: uword top_of_space =
FindTopOfSpace(heap_->isolate());
On 2017/08/02 19:20:48, rmacnak wrote:
> This will count of the unallocated space in all but the highest TLAB as
"used".
> What about starting with Scavenger::top_ - Scavenger::object_start() and
> subtracting out the unallocated space from each TLAB?

If each thread allocates at most one TLAB, then we can do this. However, it is
possible for a thread to abandon a TLAB (b/c there is not enough space to
allocate an object) and successfully allocate a new one. The former TLAB's
bounds are lost.

One way to calculate the actual size is to iterate over the entire new space and
skipping over FreelistElements. However, I believe we want UsedInWords() to be
as fast as possible.

[rmacnak, 2017-08-02 20:50:59]

lgtm

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

https://codereview.chromium.org/2992753002/diff/40001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:720: void Scavenger::MakeALLTLABsIterable(Isolate*
isolate) const {
All (lowercase Ls)

[danunez, 2017-08-08 17:50:11]

New patchset which needs review.

[danunez, 2017-08-10 17:15:17]

New patchset which adds locking and integrates the changes made from previous
TLAB fixes.

[rmacnak, 2017-08-10 18:37:07]

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

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/heap.cc#new...
runtime/vm/heap.cc:635: // TODO(danunez): Need to lock and unlock for new space
here.
This function should already be always running during a safepoint.

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

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.c...
runtime/vm/scavenger.cc:728: heap_->MakeTLABIterable(mutator_thread);
An isolate might not have a mutator_thread yet.

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.c...
runtime/vm/scavenger.cc:747: Thread* mutator_thread = isolate->mutator_thread();
An isolate might not have a mutator_thread yet.

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.c...
runtime/vm/scavenger.cc:950: Thread* current =
isolate->thread_registry()->active_list();
"active_list"  What about an unscheduled mutator thread?

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

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.h...
runtime/vm/scavenger.h:242: void FlushTLS() const;
There is no definition of FlushTLS.

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.h...
runtime/vm/scavenger.h:342: Mutex* space_lock_;
// Protects allocation of TLABs.

[danunez, 2017-08-10 20:36:46]

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

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/heap.cc#new...
runtime/vm/heap.cc:635: // TODO(danunez): Need to lock and unlock for new space
here.
On 2017/08/10 18:37:06, rmacnak wrote:
> This function should already be always running during a safepoint.

Done.

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

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.c...
runtime/vm/scavenger.cc:728: heap_->MakeTLABIterable(mutator_thread);
On 2017/08/10 18:37:07, rmacnak wrote:
> An isolate might not have a mutator_thread yet.

Right. Adding a check for NULL mutator_thread.

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.c...
runtime/vm/scavenger.cc:747: Thread* mutator_thread = isolate->mutator_thread();
On 2017/08/10 18:37:06, rmacnak wrote:
> An isolate might not have a mutator_thread yet.

Done.

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.c...
runtime/vm/scavenger.cc:950: Thread* current =
isolate->thread_registry()->active_list();
On 2017/08/10 18:37:07, rmacnak wrote:
> "active_list"  What about an unscheduled mutator thread?

Added a check for an unscheduled mutator thread.

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

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.h...
runtime/vm/scavenger.h:242: void FlushTLS() const;
On 2017/08/10 18:37:07, rmacnak wrote:
> There is no definition of FlushTLS.

Removed this declaration.

https://codereview.chromium.org/2992753002/diff/100001/runtime/vm/scavenger.h...
runtime/vm/scavenger.h:342: Mutex* space_lock_;
On 2017/08/10 18:37:07, rmacnak wrote:
> // Protects allocation of TLABs.

Done.

[rmacnak, 2017-08-10 21:16:39]

lgtm

[danunez, 2017-08-10 22:13:27]

Description was changed from

==========
Prepares allocation for proper sync with mutator and bg threads.

When top_ is required, we now search through all active threads and pull
the top_ furthest in the space.

When searching the space, we fills every thread's TLAB with a
FreeListElement. Note that future allocations can safely overwrite
them. We do NOT abandon the TLAB (i.e. set top_,end_ to 0) in these
cases.

Added notes to determing where to lock/unlock for threads

R=rmacnak@google.com,asiva@google.com
==========

to

==========
Prepares allocation for proper sync with mutator and bg threads.

When top_ is required, we now search through all active threads and pull
the top_ furthest in the space.

When searching the space, we fills every thread's TLAB with a
FreeListElement. Note that future allocations can safely overwrite
them. We do NOT abandon the TLAB (i.e. set top_,end_ to 0) in these
cases.

Added notes to determing where to lock/unlock for threads

R=asiva@google.com, rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/78745901799347a9184689b01c92a039fbd76698
==========

[danunez, 2017-08-10 22:13:28]

Committed patchset #7 (id:120001) manually as
78745901799347a9184689b01c92a039fbd76698 (presubmit successful).