Add map transition for observed objects

Add map transition for observed objects

This patch enables objects to undergo a single transition when they become observed, avoiding the need to create a new map for every observed objects.

Observed objects which become unobserved does not cause another map transition and unobserved does not clear the observed bit on the map. The unobserved object.

R=verwaest@chromium.org

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

[rossberg, 2013-07-12 06:58:02]

https://codereview.chromium.org/18221006/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/18221006/diff/2001/src/runtime.cc#newcode13744
src/runtime.cc:13744: RUNTIME_FUNCTION(MaybeObject*, Runtime_SetIsObserved) {
Drive-by comment: I think the (by now rather non-trivial) logic in this function
should be encapsulated in a method in objects.cc

[Toon Verwaest, 2013-07-12 09:02:55]

https://codereview.chromium.org/18221006/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/18221006/diff/2001/src/runtime.cc#newcode13744
src/runtime.cc:13744: RUNTIME_FUNCTION(MaybeObject*, Runtime_SetIsObserved) {
I agree.

Also, can we share this logic between SetIsObserved and JSObject::Freeze? It's
almost identical by now. The only differences (eg keeping the observed object
fast vs frozen becoming slow if adding a transition fails) are probably
irrelevant by now? Adding transitions will virtually never fail; so you can just
choose one uniform backup.

By briefly comparing this code with the JSObject::Freeze implementation, it
seems we are not doing any access checks for Object.observe? Shouldn't we do
that as well, to ensure we don't leak information?

On 2013/07/12 06:58:02, rossberg wrote:
> Drive-by comment: I think the (by now rather non-trivial) logic in this
function
> should be encapsulated in a method in objects.cc

[rafaelw, 2013-07-12 21:17:37]

https://codereview.chromium.org/18221006/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/18221006/diff/2001/src/runtime.cc#newcode13744
src/runtime.cc:13744: RUNTIME_FUNCTION(MaybeObject*, Runtime_SetIsObserved) {
On 2013/07/12 09:02:56, Toon Verwaest wrote:
> I agree.
> 
> Also, can we share this logic between SetIsObserved and JSObject::Freeze? It's
> almost identical by now. The only differences (eg keeping the observed object
> fast vs frozen becoming slow if adding a transition fails) are probably
> irrelevant by now? Adding transitions will virtually never fail; so you can
just
> choose one uniform backup.
> 
> By briefly comparing this code with the JSObject::Freeze implementation, it
> seems we are not doing any access checks for Object.observe? Shouldn't we do
> that as well, to ensure we don't leak information?
> 
> On 2013/07/12 06:58:02, rossberg wrote:
> > Drive-by comment: I think the (by now rather non-trivial) logic in this
> function
> > should be encapsulated in a method in objects.cc

Done.

[Toon Verwaest, 2013-07-12 23:15:03]

LGTM

[rafaelw, 2013-07-13 00:05:48]

Thanks.

Note the final fix which I just uploaded which avoids attempting to retrieve the
oldValue if a transition was found.

[adamk, 2013-07-13 00:20:47]

Committed patchset #5 manually as r15650 (presubmit successful).