- Add a WeakTable to the VM. This is used to remember the

- Add a WeakTable to the VM. This is used to remember the
  native peers registered through the Dart C API as well
  as assigning identity hashcodes to objects when needed.
- Use the hashcode to lookup entries in the Expando.

R=asiva@google.com

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

[siva, 2013-06-27 22:11:33]

lgtm with some comments/questions.

https://codereview.chromium.org/17992002/diff/1/runtime/lib/expando_patch.dart
File runtime/lib/expando_patch.dart (right):

https://codereview.chromium.org/17992002/diff/1/runtime/lib/expando_patch.dar...
runtime/lib/expando_patch.dart:66: // Not entering a null value. We just needed
to make sure to find
This comment seems incomplete.

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

https://codereview.chromium.org/17992002/diff/1/runtime/vm/gc_marker.cc#newco...
runtime/vm/gc_marker.cc:392: if (!raw_obj->IsMarked()) {
how does this work for deleted entries (they are considered valid by
IsValidEntryAt(i) above but table->ObjectAt(i) would return 1

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

https://codereview.chromium.org/17992002/diff/1/runtime/vm/scavenger.cc#newco...
runtime/vm/scavenger.cc:572: uword header = *reinterpret_cast<uword*>(raw_addr);
how does this work for deleted entries (they are considered valid by
IsValidEntryAt(i) above but table->ObjectAt(i) would return 1

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

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.cc#newc...
runtime/vm/weak_table.cc:48: return Rehash()->SetValue(key, val);
instead of first finding a slot to add and then realizing a rehash is needed,
rehashing and again finding a slot to add why not
trigger a rehash after adding an entry. That is you add the entry first and then
if (used >= limit() return Rehash();

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

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:32: // Reduce the capacity.
ASSERT(sz > 1);

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:36: new_sz = sz * 2;
we should protect against overflows here.
Maybe something like:

if (new_size < sz) {
  FATAL(...);
}

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:45: bool IsValidEntryAt(intptr_t i) {
const

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:60: RawObject* ObjectAt(intptr_t i) {
const
Here and some more functions below.

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:94: static intptr_t LimitFor(intptr_t size) { return 3 *
(size / 4); }
Use names constants instead of 3, 4 etc. here and above too
(e.g: 8, 2)

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:127: // Setting a value of 0 is equivalent to
invalidating the entry.
The comment here states setting a value of 0 is equivalent to invalidating the
entry... but in code above
IsValidEntryAt(i) returns true for these entries.

Maybe the comment should say setting a value of 0 is equivalent to deleting the
entry at that index.

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:148: intptr_t count_;
Missing the DISALLOW stuff...

[Ivan Posva, 2013-06-27 23:59:12]

https://codereview.chromium.org/17992002/diff/1/runtime/lib/expando_patch.dart
File runtime/lib/expando_patch.dart (right):

https://codereview.chromium.org/17992002/diff/1/runtime/lib/expando_patch.dar...
runtime/lib/expando_patch.dart:66: // Not entering a null value. We just needed
to make sure to find
On 2013/06/27 22:11:33, siva wrote:
> This comment seems incomplete.

Done.

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

https://codereview.chromium.org/17992002/diff/1/runtime/vm/gc_marker.cc#newco...
runtime/vm/gc_marker.cc:392: if (!raw_obj->IsMarked()) {
On 2013/06/27 22:11:33, siva wrote:
> how does this work for deleted entries (they are considered valid by
> IsValidEntryAt(i) above but table->ObjectAt(i) would return 1

IsValidEntryAt(i) is "return (data_[ValueIndex(i)] != 0);". The rest is just the
complicated assertion.

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

https://codereview.chromium.org/17992002/diff/1/runtime/vm/scavenger.cc#newco...
runtime/vm/scavenger.cc:572: uword header = *reinterpret_cast<uword*>(raw_addr);
On 2013/06/27 22:11:33, siva wrote:
> how does this work for deleted entries (they are considered valid by
> IsValidEntryAt(i) above but table->ObjectAt(i) would return 1

ditto.

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

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.cc#newc...
runtime/vm/weak_table.cc:48: return Rehash()->SetValue(key, val);
On 2013/06/27 22:11:33, siva wrote:
> instead of first finding a slot to add and then realizing a rehash is needed,
> rehashing and again finding a slot to add why not
> trigger a rehash after adding an entry. That is you add the entry first and
then
> if (used >= limit() return Rehash();

Done.

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

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:32: // Reduce the capacity.
On 2013/06/27 22:11:33, siva wrote:
> ASSERT(sz > 1);

sz of zero is a legal value.

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:36: new_sz = sz * 2;
On 2013/06/27 22:11:33, siva wrote:
> we should protect against overflows here.
> Maybe something like:
> 
> if (new_size < sz) {
>   FATAL(...);
> }

Done.

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:45: bool IsValidEntryAt(intptr_t i) {
On 2013/06/27 22:11:33, siva wrote:
> const

Done.

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:60: RawObject* ObjectAt(intptr_t i) {
On 2013/06/27 22:11:33, siva wrote:
> const
> Here and some more functions below.

Done.

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:94: static intptr_t LimitFor(intptr_t size) { return 3 *
(size / 4); }
On 2013/06/27 22:11:33, siva wrote:
> Use names constants instead of 3, 4 etc. here and above too
> (e.g: 8, 2)

Added a kMinSize and a comment in LimitFor, as 3 and 4 were only mentioned
there.

https://codereview.chromium.org/17992002/diff/1/runtime/vm/weak_table.h#newco...
runtime/vm/weak_table.h:148: intptr_t count_;
On 2013/06/27 22:11:33, siva wrote:
> Missing the DISALLOW stuff...

Done.

[Ivan Posva, 2013-06-28 00:01:59]

Committed patchset #2 manually as r24563 (presubmit successful).