Process weak reference sets when a scavenge invokes the API callbacks.

Process weak reference sets when a scavenge invokes the API callbacks.

BUG=2391
Committed: https://code.google.com/p/dart/source/detail?r=6348

[siva, 2012-04-06 17:12:25]

lgtm

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:248: if (!raw_obj->IsNewObject()) {
Wouldn't just this check be sufficient, it also implicitly implies something
passing this is a heap object.

[podivilov, 2012-04-06 17:20:10]

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

http://codereview.chromium.org/9956138/diff/1/runtime/vm/scavenger.cc#newcode228
runtime/vm/scavenger.cc:228: isolate->gc_prologue_callbacks().Invoke();
Please invoke epilogue callbacks here.

[Ivan Posva, 2012-04-06 21:54:06]

LGTM, but I am wondering how the tests pass if we are currently calling the
prologue code where we should be calling the epilogue as Pavel pointed out.

-Ivan

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:243: bool Scavenger::IsUnreachable(RawObject** p) {
Can you change this to Scavenger::IsReachable() to make this a bit more readable
and to avoid the double negative where this is used?

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:245: if (!raw_obj->IsHeapObject()) {
This is surprising to me to say the least. Having a Smi in the weak reference
set keeps all of the references alive? Should a Smi even be allowed here and if
it is, shouldn't it be the other way around as the object 1 will keep the set
alive, while the result of 1 * 1.0 will not.

[cshapiro, 2012-04-06 22:24:31]

The reason the tests pass is because the original behavior of the callbacks
assumed they would be invoked on every old collection.  Adding the ability to
invoke the callbacks on a new collection was a late feature that was decided on
during review.  That is probably how the absence of the weak scavenge and the
cut-and-paste error confusing prologue and epilogue came to be.

I will make the suggested fixes, update the tests, and re-upload.

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:228: isolate->gc_prologue_callbacks().Invoke();
Yes, will do.

Also, I will update the tests to exercise this functionality.

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:243: bool Scavenger::IsUnreachable(RawObject** p) {
Yes, will do.

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:245: if (!raw_obj->IsHeapObject()) {
I am not sure why this is surprising.  Smi instances are an instance of just
another subclass of Integer.  This code ensures that these objects are always
considered reachable just like 

1)         0xFFFFFFFF, a Mint on 32-bit, a Smi on 64-bit 
2) 0x7FFFFFFFFFFFFFFF, a Mint on 64-bit, a Bigint on 32-bit  
3) 0xFFFFFFFFFFFFFFFF, a Bigint

would be.

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:248: if (!raw_obj->IsNewObject()) {
Objects that become unreferenced after a new space collection are stored in the
from space part of new space.  Looking at this another way, at this point in
time, all live objects are in to space.

What this test tells us is that the object is in a part of the heap that was not
condemned by this collection and the object is guaranteed live.

What we want to know here is that the object did not survive the from-space
to-space flip.  We find that out on line 252.

[cshapiro, 2012-04-10 02:40:31]

I am going to go ahead and submit this to unblock Dartium.  I am happy to
revisit the issue of Smi reachablity in a subsequent change if the current
behavior is still believed to be undesirable.

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:243: bool Scavenger::IsUnreachable(RawObject** p) {
While this is very rational adjustment when considered in isolation, it has been
so long since I made the change that I lost the context for the naming.

The unreachability test was supposed to be shared with the handle visiting code.
 The use of Unreachable as opposed to Reachable has some consistency given the
two uses.  Anyway, I have made the fix in VisitHandle and so this looks a bit
less strange (no more isolated double negative).  If you still think this looks
backward, I can submit a clean-up that flips the naming here and in the marker.

https://chromiumcodereview.appspot.com/9956138/diff/1/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:245: if (!raw_obj->IsHeapObject()) {
I thought about this over the weekend.  Numbers are immutable and this is often
an opportunity for time and space optimizations.  Looking at it another way,
relying on values produced by different expressions being not equal is not
something that we are likely to make guarantees about.

Consider the following situations: "1.0 == 1.0" is always true, "1.0 === 1.0" is
true today under a broad range of circumstances, and "1.0 === (1 * 1.0)" is not
presently true but may become true in the future.

Reflecting on your example, Smi 1 does keep a set alive and 1.0 might as well,
depending on its provenance.

I am not sure there is an ideal resolution here.  This might be best to revisit
in a subsequent change as other parts of the code such as the handle processing
and the marker treats Smi objects as always live.