Changes new space allocation from simple bump pointer allocation from

Changes new space allocation from simple bump pointer allocation from
mutator to thread local allocation buffer system used only by mutator.

The new allocation scheme is as follows:

- Mutator allocates an aligned buffer from the heap.
- For each object the mutator wishes to allocate
  - If the object fits in the overall space of the bufffer,
    bump allocate into the buffer.
  - If the object is larger than the buffer overall, allocate into old
    space.
  - If the object is not larger than the buffer but cannot fit,
    make the buffer iterable by GC and allocate a new buffer.
  - If the mutator cannot get a new buffer, do a GC and try again.
  - If the mutator still cannot get a new buffer, allocate the object
    in old space.
  - If the mutator does get a new buffer (before or after GC), allocate
    the object into the buffer.

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

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

[danunez, 2017-07-25 17:15:22]

Second part of the TLAB change.

[rmacnak, 2017-07-25 21:02:45]

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

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode60
runtime/vm/heap.cc:60: void Heap::MakeTLABIterable(uword start, uword end) {
Consider recasting as Scavenger::AbandonRemainingTLAB(Thread* thread) for
symmetry with PageSpace::AbandonBumpAllocation.

This also looks like a good place to set Thread's top/end to indicate it does
not currently have a TLAB.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode62
runtime/vm/heap.cc:62: intptr_t size = end - start;
Assert size is a multiple of kObjectAlignment

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode66
runtime/vm/heap.cc:66: if (size > kObjectAlignment) {
Create a FreeListElement for size == kObjectAlignment too. The only special case
should be size == 0.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode103
runtime/vm/heap.cc:103: thread->set_end(new_space_.top());
tlab_top + tlab_size

(Thinking forward, new_space_.top() may be out of date if another thread
allocates a TLAB)

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode111
runtime/vm/heap.cc:111: if (addr == 0) {
addr must be 0 if we reach here, right?

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode123
runtime/vm/heap.cc:123: thread->set_end(new_space_.top());
tlab_top + tlab_size

This thread setup seems like it should be pushed into TryAllocateNewTLAB instead
of repeated.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode127
runtime/vm/heap.cc:127: // In that case, we must fail through and allocate into
old space.
fall through

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode608
runtime/vm/heap.cc:608: uword top_bk = new_space_.FlushTLS();
Consider saved_top since saved_x is the commonly used name in our codebase.

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

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2559: mutator_thread_ = thread;
Consider lazy allocation of the TLAB.

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

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:410: thread->set_top(top_);
Can this use zeroes?

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:716: uword top_bk = top_;
Throughout this file: saved_top

[danunez, 2017-07-26 20:19:21]

New patchset incoming.

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

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode60
runtime/vm/heap.cc:60: void Heap::MakeTLABIterable(uword start, uword end) {
On 2017/07/25 21:02:44, rmacnak wrote:
> Consider recasting as Scavenger::AbandonRemainingTLAB(Thread* thread) for
> symmetry with PageSpace::AbandonBumpAllocation.
> 
> This also looks like a good place to set Thread's top/end to indicate it does
> not currently have a TLAB.

Good call.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode62
runtime/vm/heap.cc:62: intptr_t size = end - start;
On 2017/07/25 21:02:44, rmacnak wrote:
> Assert size is a multiple of kObjectAlignment

Done.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode66
runtime/vm/heap.cc:66: if (size > kObjectAlignment) {
On 2017/07/25 21:02:44, rmacnak wrote:
> Create a FreeListElement for size == kObjectAlignment too. The only special
case
> should be size == 0.

Done.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode103
runtime/vm/heap.cc:103: thread->set_end(new_space_.top());
On 2017/07/25 21:02:44, rmacnak wrote:
> tlab_top + tlab_size
> 
> (Thinking forward, new_space_.top() may be out of date if another thread
> allocates a TLAB)

That is a very good point, especially since that section would not be locked in
the future.

I will also add the following assertion

ASSERT(tlab_top + tlab_size == new_space_.top())

Addressing the future comment, I will push this setup into TryAllocateNewTLAB.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode111
runtime/vm/heap.cc:111: if (addr == 0) {
On 2017/07/25 21:02:45, rmacnak wrote:
> addr must be 0 if we reach here, right?

Correct. Consider that removed.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode123
runtime/vm/heap.cc:123: thread->set_end(new_space_.top());
On 2017/07/25 21:02:44, rmacnak wrote:
> tlab_top + tlab_size
> 
> This thread setup seems like it should be pushed into TryAllocateNewTLAB
instead
> of repeated.

Done.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode127
runtime/vm/heap.cc:127: // In that case, we must fail through and allocate into
old space.
On 2017/07/25 21:02:44, rmacnak wrote:
> fall through

Done.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/heap.cc#newcode608
runtime/vm/heap.cc:608: uword top_bk = new_space_.FlushTLS();
On 2017/07/25 21:02:44, rmacnak wrote:
> Consider saved_top since saved_x is the commonly used name in our codebase.

Done.

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

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2559: mutator_thread_ = thread;
On 2017/07/25 21:02:45, rmacnak wrote:
> Consider lazy allocation of the TLAB.

Done.

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

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:410: thread->set_top(top_);
On 2017/07/25 21:02:45, rmacnak wrote:
> Can this use zeroes?

Good call. Found a latent error by changing this to 0. Will be doing that in the
future.

https://codereview.chromium.org/2985863002/diff/1/runtime/vm/scavenger.cc#new...
runtime/vm/scavenger.cc:716: uword top_bk = top_;
On 2017/07/25 21:02:45, rmacnak wrote:
> Throughout this file: saved_top

Done.

[rmacnak, 2017-07-26 21:30:30]

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

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:72: ASSERT(start + size == new_space_.top());
(start + size) == new_space_.top()

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:93: if (tlab_size > 0 && size > tlab_size) {
if ((tlab_size > 0) && (size > tlab_size)) {

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:106: 
ASSERT(thread has no active tlab)

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

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2577: if (Dart::vm_isolate() != NULL && heap() !=
Dart::vm_isolate()->heap()) {
if ((Dart::vm_isolate() != NULL) && (heap() != Dart::vm_isolate()->heap())) {

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

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:818: if (thread->IsMutatorThread() &&
isolate->IsMutatorThreadScheduled()) {
Re: thread->IsMutatorThread()

What if the GC was started by a background thread, in which case
Thread::Current() != isolate->mutator_thread()? The mutator thread's tlab would
still need to be cleared.

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

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:166: ASSERT(thread->end() == 0 || thread->end() == top_);
ASSERT((thread->end() == 0) || (thread->end() == top_));

[danunez, 2017-07-26 22:07:12]

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

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:72: ASSERT(start + size == new_space_.top());
On 2017/07/26 21:30:29, rmacnak wrote:
> (start + size) == new_space_.top()

Done.

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:93: if (tlab_size > 0 && size > tlab_size) {
On 2017/07/26 21:30:29, rmacnak wrote:
> if ((tlab_size > 0) && (size > tlab_size)) {

Done.

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:106: 
On 2017/07/26 21:30:29, rmacnak wrote:
> ASSERT(thread has no active tlab)

Done.

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

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2577: if (Dart::vm_isolate() != NULL && heap() !=
Dart::vm_isolate()->heap()) {
On 2017/07/26 21:30:30, rmacnak wrote:
> if ((Dart::vm_isolate() != NULL) && (heap() != Dart::vm_isolate()->heap())) {

Done.

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

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:818: if (thread->IsMutatorThread() &&
isolate->IsMutatorThreadScheduled()) {
On 2017/07/26 21:30:30, rmacnak wrote:
> Re: thread->IsMutatorThread()
> 
> What if the GC was started by a background thread, in which case
> Thread::Current() != isolate->mutator_thread()? The mutator thread's tlab
would
> still need to be cleared.

Right. Now modified to grab the mutator thread and abandon its TLAB.

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

https://codereview.chromium.org/2985863002/diff/20001/runtime/vm/scavenger.h#...
runtime/vm/scavenger.h:166: ASSERT(thread->end() == 0 || thread->end() == top_);
On 2017/07/26 21:30:30, rmacnak wrote:
> ASSERT((thread->end() == 0) || (thread->end() == top_));

Done.

[rmacnak, 2017-07-27 20:33:44]

lgtm

[danunez, 2017-07-27 20:53:44]

Description was changed from

==========
Changes new space allocation from simple bump pointer allocation from
mutator to thread local allocation buffer system used only by mutator.

The new allocation scheme is as follows:

- Mutator allocates an aligned buffer from the heap.
- For each object the mutator wishes to allocate
  - If the object fits in the overall space of the bufffer,
    bump allocate into the buffer.
  - If the object is larger than the buffer overall, allocate into old
    space.
  - If the object is not larger than the buffer but cannot fit,
    make the buffer iterable by GC and allocate a new buffer.
  - If the mutator cannot get a new buffer, do a GC and try again.
  - If the mutator still cannot get a new buffer, allocate the object
    in old space.
  - If the mutator does get a new buffer (before or after GC), allocate
    the object into the buffer.

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

to

==========
Changes new space allocation from simple bump pointer allocation from
mutator to thread local allocation buffer system used only by mutator.

The new allocation scheme is as follows:

- Mutator allocates an aligned buffer from the heap.
- For each object the mutator wishes to allocate
  - If the object fits in the overall space of the bufffer,
    bump allocate into the buffer.
  - If the object is larger than the buffer overall, allocate into old
    space.
  - If the object is not larger than the buffer but cannot fit,
    make the buffer iterable by GC and allocate a new buffer.
  - If the mutator cannot get a new buffer, do a GC and try again.
  - If the mutator still cannot get a new buffer, allocate the object
    in old space.
  - If the mutator does get a new buffer (before or after GC), allocate
    the object into the buffer.

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

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

[danunez, 2017-07-27 20:53:45]

Committed patchset #3 (id:40001) manually as
e6e378bb6eb5ba90f890399a889b8daad8418c45 (presubmit successful).