Object.observe: generate change records for named properties. (issue 11347037)

[ross...@chromium.org]

Reviewers: Toon Verwaest,

Description:
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.

Object.observe: implement map bit and functions to access it.


R=verw...@chromium.org
BUG=


Please review this at https://codereview.chromium.org/11347037/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/bootstrapper.cc
M src/contexts.h
M src/flag-definitions.h
M src/ic.cc
M src/object-observe.js
M src/objects-inl.h
M src/objects.h
M src/objects.cc
M src/runtime.h
M src/runtime.cc
M src/v8natives.js
M test/mjsunit/harmony/object-observe.js

[raf...@chromium.org]

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.

https://codereview.chromium.org/11347037/

[raf...@chromium.org]

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?

https://codereview.chromium.org/11347037/

[raf...@chromium.org]

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.


https://codereview.chromium.org/11347037/

[verw...@chromium.org]

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#newcode1680
src/objects.cc:1680: MaybeObject* ret;
Can we call this maybe_result for consistency?

https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc#newcode1709
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#newcode1720
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#newcode7620
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#newcode13240
src/runtime.cc:13240: if (!maybe->To<Map>(&map)) return maybe;
The <Map> is not needed; maybe->To(&map) works just fine.

https://chromiumcodereview.appspot.com/11347037/

[ross...@chromium.org]

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.

https://codereview.chromium.org/11347037/

[verw...@chromium.org]

lgtm



http://codereview.chromium.org/11347037/

[raf...@chromium.org]

lgtm

https://codereview.chromium.org/11347037/

[ross...@chromium.org]

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

https://codereview.chromium.org/11347037/