Fix external size accounting for prologue weak persistent handles.

Fix external size accounting for prologue weak persistent handles.

Include prologue weak persistent handles (PWPH) when checking for promotion.

Since pointers have already been updated for PWPHs, use a bit in the handle to
remember whether the referent used to be in new space.

R=asiva@google.com

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

[siva, 2014-03-11 16:35:01]

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2273: EXPECT(heap->ExternalInWords(Heap::kNew)
== kWeakExternalSize / kWordSize);
EXPECT(heap->ExternalInWords(Heap::kOld) == 0);

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

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:235: Heap::Space before = WasPromotedSinceLastCall()
? Heap::kNew : after;
Since we only promote new to old why can't this check just be a simple
if (IsSetNewSpaceBit() && (SpaceForExternal() == Heap::kOld)) {
  FreeExternal(external_size(), Heap::kNew);
  AllocateExternal(external_size(), Heap::kOld);
  ResetExternalNewSpaceBit();
}

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:237:
isolate->heap()->AllocateExternal(external_size(), after);
if before == after it not necessary to do this Free/Allocate stuff right?

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:326: }
The use of UpdateExternalNewSpaceBit to both set and reset the bit makes it hard
to read, why not have explicit:

SetExternalNewSpaceBit() { ... }
ResetExternalNewSpaceBit() { ... }

SetExternalNewSpaceBit is called in SetExternalSize() and
ResetExternalNewSpaceBit is called in WasPromotedSizeLastCall();

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

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/scavenger.cc#...
runtime/vm/scavenger.cc:711: IterateWeakRoots(isolate, &weak_visitor, true);
Why did you set the parameter 'visit_prologue_weak_persistent_handles' to true
unconditionally in this call to IterateWeakRoots?

[koda, 2014-03-11 17:35:45]

PTAL

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:2273: EXPECT(heap->ExternalInWords(Heap::kNew)
== kWeakExternalSize / kWordSize);
On 2014/03/11 16:35:01, siva wrote:
> EXPECT(heap->ExternalInWords(Heap::kOld) == 0);

Done.

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

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:235: Heap::Space before = WasPromotedSinceLastCall()
? Heap::kNew : after;
On 2014/03/11 16:35:01, siva wrote:
> Since we only promote new to old why can't this check just be a simple
> if (IsSetNewSpaceBit() && (SpaceForExternal() == Heap::kOld)) {
>   FreeExternal(external_size(), Heap::kNew);
>   AllocateExternal(external_size(), Heap::kOld);
>   ResetExternalNewSpaceBit();
> }

Done.

My reasoning was to localize the assumption that we have exactly two generations
and make the code as general as possible, but in retrospect, that might be more
trouble than it's worth.

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:237:
isolate->heap()->AllocateExternal(external_size(), after);
On 2014/03/11 16:35:01, siva wrote:
> if before == after it not necessary to do this Free/Allocate stuff right?

Done.

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/dart_api_stat...
runtime/vm/dart_api_state.h:326: }
On 2014/03/11 16:35:01, siva wrote:
> The use of UpdateExternalNewSpaceBit to both set and reset the bit makes it
hard
> to read, why not have explicit:
> 
> SetExternalNewSpaceBit() { ... }
> ResetExternalNewSpaceBit() { ... }
> 
> SetExternalNewSpaceBit is called in SetExternalSize() and
> ResetExternalNewSpaceBit is called in WasPromotedSizeLastCall();

Done.

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

https://codereview.chromium.org/193473002/diff/40001/runtime/vm/scavenger.cc#...
runtime/vm/scavenger.cc:711: IterateWeakRoots(isolate, &weak_visitor, true);
On 2014/03/11 16:35:01, siva wrote:
> Why did you set the parameter 'visit_prologue_weak_persistent_handles' to true
> unconditionally in this call to IterateWeakRoots?

Because we must visit the prologue weak persistent handles to detect any
promotions. That was the core issue behind the performance degradation: that
UpdateRelocated was not being called for pwphs, causing incorrect accounting and
scavenge on every alloc.

The change to store a bit in the handle is a consequence, since at this point,
the ScavengerVisitor has already updated the raw_ pointers of the prologue
weaks, i.e., we are not observing the promotion as it happens (IsUnreachable),
as we could do for the non-prologue ones.

I added a brief comment, and named the flag, for clarity. Let me know if you
think it needs more.

[siva, 2014-03-11 18:20:37]

lgtm

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

https://codereview.chromium.org/193473002/diff/80001/runtime/vm/scavenger.cc#...
runtime/vm/scavenger.cc:703: bool prologue_weak_are_strong =
!invoke_api_callbacks;
const bool prologue_weak_are_strong = ....

https://codereview.chromium.org/193473002/diff/80001/runtime/vm/scavenger.cc#...
runtime/vm/scavenger.cc:714: bool visit_prologue_weak_handles = true;
const bool visit_prologue_weak_handles = true;

This is the style that Srdjan has recommended we use.

[koda, 2014-03-11 20:20:12]

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

https://codereview.chromium.org/193473002/diff/80001/runtime/vm/scavenger.cc#...
runtime/vm/scavenger.cc:703: bool prologue_weak_are_strong =
!invoke_api_callbacks;
On 2014/03/11 18:20:37, siva wrote:
> const bool prologue_weak_are_strong = ....

Done.

https://codereview.chromium.org/193473002/diff/80001/runtime/vm/scavenger.cc#...
runtime/vm/scavenger.cc:714: bool visit_prologue_weak_handles = true;
On 2014/03/11 18:20:37, siva wrote:
> const bool visit_prologue_weak_handles = true;
> 
> This is the style that Srdjan has recommended we use.

Done.

[koda, 2014-03-11 20:27:47]

Committed patchset #6 manually as r33552 (presubmit successful).