Account Options

  1. Sign in
The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Google Groups Home
« Groups Home
v8-dev
Home
+ new post
About this group
Join this group
Object.observe: generate change records for named properties. (issue 11347037)
Options
There are currently too many topics in this group that display first. To make this topic appear first, remove this option from another topic.
There was an error processing your request. Please try again.
flag
   9 messages - Collapse all  -  Translate all to Translated (View all originals)
The group you are posting to is a Usenet group. Messages posted to this group will make your email address visible to anyone on the Internet.
Your reply message has not been sent.
Your post was successful
 
From:
To:
Cc:
Followup To:
Add Cc | Add Followup-to | Edit Subject
Subject:
Validation:
For verification purposes please type the characters you see in the picture below or the numbers you hear by clicking the accessibility icon. Listen and type the numbers you hear
 
rossb...@chromium.org  
View profile  
 More options Oct 30 2012, 8:40 am
From: rossb...@chromium.org
Date: Tue, 30 Oct 2012 12:40:48 +0000
Local: Tues, Oct 30 2012 8:40 am
Subject: Object.observe: generate change records for named properties. (issue 11347037)
Print | Individual message | Show original | Report this message | Find messages by this author
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=verwa...@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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
rafa...@chromium.org  
View profile  
 More options Oct 31 2012, 10:44 am
From: rafa...@chromium.org
Date: Wed, 31 Oct 2012 14:44:31 +0000
Local: Wed, Oct 31 2012 10:44 am
Subject: Re: Object.observe: generate change records for named properties. (issue 11347037)
Print | Individual message | Show original | Report this message | Find messages by this author

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...
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...
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...
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#newcod...
src/objects.cc:1699: JSFunction::cast(value),
whitespace

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcod...
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#newcod...
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#newcod...
src/objects.cc:2990: return ret;
same question about returning MaybeObject after invoking script.

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

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

https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcod...
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#newcode...
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...

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

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
rafa...@chromium.org  
View profile  
 More options Oct 31 2012, 1:27 pm
From: rafa...@chromium.org
Date: Wed, 31 Oct 2012 17:27:24 +0000
Local: Wed, Oct 31 2012 1:27 pm
Subject: Re: Object.observe: generate change records for named properties. (issue 11347037)
Print | Individual message | Show original | Report this message | Find messages by this author

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#newcode...
src/objects.h:2279: void NotifyObservers(const char* type, String* name,
Object* oldValue);
also, are arguments unix_hacker style?

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
rafa...@chromium.org  
View profile  
 More options Nov 2 2012, 11:57 am
From: rafa...@chromium.org
Date: Fri, 02 Nov 2012 15:57:36 +0000
Local: Fri, Nov 2 2012 11:57 am
Subject: Re: Object.observe: generate change records for named properties. (issue 11347037)
Print | Individual message | Show original | Report this message | Find messages by this author
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
verwa...@chromium.org  
View profile  
 More options Nov 5 2012, 8:33 am
From: verwa...@chromium.org
Date: Mon, 05 Nov 2012 13:33:22 +0000
Local: Mon, Nov 5 2012 8:33 am
Subject: Re: Object.observe: generate change records for named properties. (issue 11347037)
Print | Individual message | Show original | Report this message | Find messages by this author
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...
src/objects.cc:1680: MaybeObject* ret;
Can we call this maybe_result for consistency?

https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc...
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...
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...
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#...
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...
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
rossb...@chromium.org  
View profile  
 More options Nov 5 2012, 12:11 pm
From: rossb...@chromium.org
Date: Mon, 05 Nov 2012 17:11:08 +0000
Local: Mon, Nov 5 2012 12:11 pm
Subject: Re: Object.observe: generate change records for named properties. (issue 11347037)
Print | Individual message | Show original | Report this message | Find messages by this author

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...
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...
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...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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#newcod...
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#newcode...
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#newcode...
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#newcode...
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...

> 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#newcod...
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
verwa...@chromium.org  
View profile  
 More options Nov 6 2012, 5:34 am
From: verwa...@chromium.org
Date: Tue, 06 Nov 2012 10:34:52 +0000
Local: Tues, Nov 6 2012 5:34 am
Subject: Re: Object.observe: generate change records for named properties. (issue 11347037)
Print | Individual message | Show original | Report this message | Find messages by this author

 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
rafa...@chromium.org  
View profile  
 More options Nov 6 2012, 5:46 am
From: rafa...@chromium.org
Date: Tue, 06 Nov 2012 10:46:21 +0000
Local: Tues, Nov 6 2012 5:46 am
Subject: Re: Object.observe: generate change records for named properties. (issue 11347037)
Print | Individual message | Show original | Report this message | Find messages by this author

 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
rossb...@chromium.org  
View profile  
 More options Nov 6 2012, 11:53 am
From: rossb...@chromium.org
Date: Tue, 06 Nov 2012 16:53:00 +0000
Subject: Re: Object.observe: generate change records for named properties. (issue 11347037)
Print | Individual message | Show original | Report this message | Find messages by this author
On 2012/11/02 15:57:36, rafaelw wrote:

path.

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

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
End of messages
« Back to Discussions « Newer topic     Older topic »

Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
©2013 Google