Object.observe: generate change records for named properties.

Object.observe: generate change records for named properties.

In more detail:
- Set observation bit for observed objects (and make NormalizedMapCache respect it).
- Mutation of observed objects is always delegated from ICs to runtime.
- Introduce JS runtime function for notifying generated changes.
- Invoke this function in the appropriate places (including some local refactoring).
- Inclusion of oldValue field is not yet implemented, nor element properties.

Also, shortened flag to --harmony-observation.


R=verwaest@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=12867

[rafaelw, 2012-10-31 14:44:31]

https://codereview.chromium.org/11347037/diff/1/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode73
src/object-observe.js:73: }
do we have JS debug-only ASSERTS? If so, it'd be nice to ASSERT(%IsObserved)

https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode115
src/object-observe.js:115: if (IS_UNDEFINED(objectInfo)) return;
Seems like we should CHECK() here - can we crash from JS? This should never
happen, right?

https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode118
src/object-observe.js:118: if (arguments.length == 4) changeRecord.oldValue =
oldValue;
For safety, I think that you should switch on arguments.length and create one of
two object literals. $Object is theoritically exposed and user script could have
installed an accessor on Object.prototype.oldValue.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1699
src/objects.cc:1699: JSFunction::cast(value),
whitespace

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1720
src/objects.cc:1720: return ret;
Isn't this unsafe? The notifyObservers could have called GC.  Don't you need to:

if (ret->IsFailure()) return ret;
Handle<Object> return_handle(return_value->ToObjectUnchecked());
// call NotifyObservers
return *return_handle;

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1732
src/objects.cc:1732: if (oldValue_raw->IsTheHole()) {
nit: This can be cleaner if you make argv a ScopedVector<Handle<Object> >, which
contains either 3 or 4 values based on whether oldValue_raw->IsTheHole().

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode2990
src/objects.cc:2990: return ret;
same question about returning MaybeObject after invoking script.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode3121
src/objects.cc:3121: return ret;
same question about returning MaybeObject

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode4069
src/objects.cc:4069: return ret;
ditto

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode4716
src/objects.cc:4716: return ret;
ditto

https://codereview.chromium.org/11347037/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11347037/diff/1/src/objects.h#newcode2279
src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object*
oldValue);
suggestion: the name "NotifyObservers" is a bit misleading here because
observers don't get notified via this function.

If you look at our new "working" branch:

https://github.com/rafaelw/v8/tree/object-observe2

We added something similar:

https://github.com/rafaelw/v8/blob/object-observe2/src/object-observe.cc#L49

But called it EnqueueChangeRecord. This name matches the spec text.

[rafaelw, 2012-10-31 17:27:24]

https://codereview.chromium.org/11347037/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11347037/diff/1/src/objects.h#newcode2279
src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object*
oldValue);
also, are arguments unix_hacker style?

[rafaelw, 2012-11-02 15:57:36]

Also, FYI.

It looks to me like setting elements in a loop is still being cached. The
following test fails with this patch applied to our branch, but passes if i turn
off IC altogether:

// Observed not IC'd.
reset();
var obj = {};
Object.observe(obj, observer.callback);
for (var i = 0; i < 3; i++) {
  obj[i] = i;
}
Object.deliverChangeRecords(observer.callback);
observer.assertCallbackRecords([
  { object: obj, name: "0", type: "new" },
  { object: obj, name: "1", type: "new" },
  { object: obj, name: "2", type: "new" },
]);

Note that if the assignment in the look is obj['foo' + i], then it passes --
thus my suspicion about elements being cached somehow through a different path.

[Toon Verwaest, 2012-11-05 13:33:22]

First round of comments.

https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc#newcode...
src/objects.cc:1680: MaybeObject* ret;
Can we call this maybe_result for consistency?

https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc#newcode...
src/objects.cc:1709: if (!ret->ToObject(&obj)) return ret;
if (ret->IsFailure()) return ret;

I actually prefer:
MaybeObject* maybe_failure = NormalizeProperties(...;
if (maybe_failure->IsFailure()) return maybe_failure;

since ret will be overwritten in the next line anyway. The values seem
independent.

https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc#newcode...
src/objects.cc:1720: return ret;
Yes. We should not jump into handlified code from non-handlified code. This is
very prone to errors, especially since the caller of the function does not know
that handlified code may cause GC without returning failure, potentially
invalidating all raw pointers in the client code.

Either we have to ensure that no code is handlified for implementing
NotifyObservers, or we'll have to pull it up to the handlified client of this
functionality.

On 2012/10/31 14:44:31, rafaelw wrote:
> Isn't this unsafe? The notifyObservers could have called GC.  Don't you need
to:
> 
> if (ret->IsFailure()) return ret;
> Handle<Object> return_handle(return_value->ToObjectUnchecked());
> // call NotifyObservers
> return *return_handle;

https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc#newcode...
src/objects.cc:7620: (other->bit_field3() & IsObserved::kMask) &&
Can we just use is_observed() == other->is_observed()?

https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.h#newcode2279
src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object*
oldValue);
Yes, use old_value in the C++ code. Also in other files.

On 2012/10/31 17:27:24, rafaelw wrote:
> also, are arguments unix_hacker style?

https://chromiumcodereview.appspot.com/11347037/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/11347037/diff/1/src/runtime.cc#newcode...
src/runtime.cc:13240: if (!maybe->To<Map>(&map)) return maybe;
The <Map> is not needed; maybe->To(&map) works just fine.

[rossberg, 2012-11-05 17:11:08]

https://codereview.chromium.org/11347037/diff/1/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode73
src/object-observe.js:73: }
On 2012/10/31 14:44:31, rafaelw wrote:
> do we have JS debug-only ASSERTS? If so, it'd be nice to ASSERT(%IsObserved)

Unfortunately, no.

https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode115
src/object-observe.js:115: if (IS_UNDEFINED(objectInfo)) return;
On 2012/10/31 14:44:31, rafaelw wrote:
> Seems like we should CHECK() here - can we crash from JS? This should never
> happen, right?

There is no direct way to do that, but I just removed the line, so that the
access below would throw an exception. Then the !threw assertion in
JSObject::Notify would fire.

https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode118
src/object-observe.js:118: if (arguments.length == 4) changeRecord.oldValue =
oldValue;
On 2012/10/31 14:44:31, rafaelw wrote:
> For safety, I think that you should switch on arguments.length and create one
of
> two object literals. $Object is theoritically exposed and user script could
have
> installed an accessor on Object.prototype.oldValue.

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1680
src/objects.cc:1680: MaybeObject* ret;
On 2012/11/05 13:33:22, Toon Verwaest wrote:
> Can we call this maybe_result for consistency?

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1699
src/objects.cc:1699: JSFunction::cast(value),
On 2012/10/31 14:44:31, rafaelw wrote:
> whitespace

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1709
src/objects.cc:1709: if (!ret->ToObject(&obj)) return ret;
On 2012/11/05 13:33:22, Toon Verwaest wrote:
> if (ret->IsFailure()) return ret;
> 
> I actually prefer:
> MaybeObject* maybe_failure = NormalizeProperties(...;
> if (maybe_failure->IsFailure()) return maybe_failure;
> 
> since ret will be overwritten in the next line anyway. The values seem
> independent.

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1720
src/objects.cc:1720: return ret;
On 2012/10/31 14:44:31, rafaelw wrote:
> Isn't this unsafe? The notifyObservers could have called GC.  Don't you need
to:
> 
> if (ret->IsFailure()) return ret;
> Handle<Object> return_handle(return_value->ToObjectUnchecked());
> // call NotifyObservers
> return *return_handle;

Indeed.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1720
src/objects.cc:1720: return ret;
On 2012/11/05 13:33:22, Toon Verwaest wrote:
> Yes. We should not jump into handlified code from non-handlified code. This is
> very prone to errors, especially since the caller of the function does not
know
> that handlified code may cause GC without returning failure, potentially
> invalidating all raw pointers in the client code.
> 
> Either we have to ensure that no code is handlified for implementing
> NotifyObservers, or we'll have to pull it up to the handlified client of this
> functionality.

Most of the methods in question are already documented as "can cause GC", and I
added respective comments to the remaining ones (plus, checked all their call
sites).

As discussed offline, there is no real alternative short of "handlifying all the
code" -- which I agree we should do at some point.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1732
src/objects.cc:1732: if (oldValue_raw->IsTheHole()) {
On 2012/10/31 14:44:31, rafaelw wrote:
> nit: This can be cleaner if you make argv a ScopedVector<Handle<Object> >,
which
> contains either 3 or 4 values based on whether oldValue_raw->IsTheHole().

Well, that would make the code pretty verbose as well. I removed the duplication
using a simpler trick.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode2990
src/objects.cc:2990: return ret;
On 2012/10/31 14:44:31, rafaelw wrote:
> same question about returning MaybeObject after invoking script.

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode3121
src/objects.cc:3121: return ret;
On 2012/10/31 14:44:31, rafaelw wrote:
> same question about returning MaybeObject

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode4069
src/objects.cc:4069: return ret;
On 2012/10/31 14:44:31, rafaelw wrote:
> ditto

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode4716
src/objects.cc:4716: return ret;
On 2012/10/31 14:44:31, rafaelw wrote:
> ditto

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode7620
src/objects.cc:7620: (other->bit_field3() & IsObserved::kMask) &&
On 2012/11/05 13:33:22, Toon Verwaest wrote:
> Can we just use is_observed() == other->is_observed()?

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11347037/diff/1/src/objects.h#newcode2279
src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object*
oldValue);
On 2012/11/05 13:33:22, Toon Verwaest wrote:
> Yes, use old_value in the C++ code. Also in other files.
> 
> On 2012/10/31 17:27:24, rafaelw wrote:
> > also, are arguments unix_hacker style?
> 

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.h#newcode2279
src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object*
oldValue);
On 2012/10/31 17:27:24, rafaelw wrote:
> also, are arguments unix_hacker style?

Done.

https://codereview.chromium.org/11347037/diff/1/src/objects.h#newcode2279
src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object*
oldValue);
On 2012/10/31 14:44:31, rafaelw wrote:
> suggestion: the name "NotifyObservers" is a bit misleading here because
> observers don't get notified via this function.
> 
> If you look at our new "working" branch:
> 
> https://github.com/rafaelw/v8/tree/object-observe2
> 
> We added something similar:
> 
> https://github.com/rafaelw/v8/blob/object-observe2/src/object-observe.cc#L49
> 
> But called it EnqueueChangeRecord. This name matches the spec text.

Done.

https://codereview.chromium.org/11347037/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11347037/diff/1/src/runtime.cc#newcode13240
src/runtime.cc:13240: if (!maybe->To<Map>(&map)) return maybe;
On 2012/11/05 13:33:22, Toon Verwaest wrote:
> The <Map> is not needed; maybe->To(&map) works just fine.

Done.

[Toon Verwaest, 2012-11-06 10:34:52]

lgtm

[rafaelw, 2012-11-06 10:46:21]

lgtm

[rossberg, 2012-11-06 16:52:59]

On 2012/11/02 15:57:36, rafaelw wrote:
> Also, FYI.
> 
> It looks to me like setting elements in a loop is still being cached. The
> following test fails with this patch applied to our branch, but passes if i
turn
> off IC altogether:
> 
> // Observed not IC'd.
> reset();
> var obj = {};
> Object.observe(obj, observer.callback);
> for (var i = 0; i < 3; i++) {
>   obj[i] = i;
> }
> Object.deliverChangeRecords(observer.callback);
> observer.assertCallbackRecords([
>   { object: obj, name: "0", type: "new" },
>   { object: obj, name: "1", type: "new" },
>   { object: obj, name: "2", type: "new" },
> ]);
> 
> Note that if the assignment in the look is obj['foo' + i], then it passes --
> thus my suspicion about elements being cached somehow through a different
path.

Ack. I'll address that in a separate CL.