Chromium Code Reviews
Help | Chromium Project | Sign in
(855)

Issue 11274014: Store Object.observe state per-isolate rather than per-context (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by adamk
Modified:
1 year, 5 months ago
CC:
v8-dev_googlegroups.com
Visibility:
Public.

Description

Store Object.observe state per-isolate rather than per-context

This requires adding a new JSObject to the strong root list and populating it from
object-observe.js. The main other change is that we now directly use ObjectHashTable
from JS rather than using WeakMap, since using the latter would end up leaking whichever
Context initialized that observation state.

Added a test via the API showing that different contexts all end up working on the same state.

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : Restructured #

Patch Set 3 : Renaming #

Patch Set 4 : Added test #

Patch Set 5 : Merged #

Patch Set 6 : More tests #

Total comments: 1

Patch Set 7 : Use ObjectHashTable directly #

Total comments: 2

Patch Set 8 : Simplified #

Total comments: 10

Patch Set 9 : Added back runtime asserts #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -38 lines) Lint Patch
M include/v8.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments 0 errors Download
M src/heap.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments ? errors Download
M src/heap.cc View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments ? errors Download
M src/object-observe.js View 1 2 3 4 5 6 7 8 1 chunk +21 lines, -11 lines 0 comments ? errors Download
M src/objects.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M src/objects-inl.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments 0 errors Download
M src/runtime.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments 0 errors Download
M src/runtime.cc View 1 2 3 4 5 6 7 8 1 chunk +43 lines, -0 lines 0 comments 0 errors Download
M test/cctest/cctest.gyp View 1 2 3 1 chunk +1 line, -0 lines 0 comments ? errors Download
A + test/cctest/test-object-observe.cc View 1 2 3 4 5 6 7 8 1 chunk +48 lines, -25 lines 0 comments 1 errors Download
Trybot results:
Commit:

Messages

Total messages: 11
adamk
First look at this, no tests yet
1 year, 5 months ago #1
rafaelw
https://codereview.chromium.org/11274014/diff/1/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/11274014/diff/1/src/object-observe.js#newcode54 src/object-observe.js:54: suggestion: var state = %GetObjectObservationState(); if (IS_UNDEFINED(state.observerInfoMap)) { state.observerInfoMap ...
1 year, 5 months ago #2
adamk
https://codereview.chromium.org/11274014/diff/1/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/11274014/diff/1/src/object-observe.js#newcode54 src/object-observe.js:54: On 2012/10/24 15:21:35, rafaelw wrote: > suggestion: > > ...
1 year, 5 months ago #3
adamk
+mstarzinger to comment on usage of per-isolate state. He suggested we need to restructure this ...
1 year, 5 months ago #4
Michael Starzinger
Drive-by-comments: As discussed offline with Andreas, I think we will need a separate "InternalWeakMap" that ...
1 year, 5 months ago #5
adamk
Minimal approach: use ObjectHashTable directly from object-observe.js via a few helper runtime functions. Pretty ugly, ...
1 year, 5 months ago #6
rossberg
Mostly looks good to me, but I'll let Michael stamp it. https://codereview.chromium.org/11274014/diff/18001/src/objects-inl.h File src/objects-inl.h (right): ...
1 year, 5 months ago #7
adamk
https://codereview.chromium.org/11274014/diff/18001/src/objects-inl.h File src/objects-inl.h (right): https://codereview.chromium.org/11274014/diff/18001/src/objects-inl.h#newcode730 src/objects-inl.h:730: // TODO: Is this sufficient? On 2012/11/06 12:27:09, rossberg ...
1 year, 5 months ago #8
Michael Starzinger
This is already looking good. Just a few comments. https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc File src/runtime.cc (right): https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc#newcode13234 src/runtime.cc:13234: ...
1 year, 5 months ago #9
adamk
https://codereview.chromium.org/11274014/diff/20003/src/heap.h File src/heap.h (right): https://codereview.chromium.org/11274014/diff/20003/src/heap.h#newcode159 src/heap.h:159: V(JSObject, object_observation_state, ObjectObservationState) Note, renamed this to just "observation_state" ...
1 year, 5 months ago #10
Michael Starzinger
1 year, 5 months ago #11
LGTM. I'll land this for you.

https://codereview.chromium.org/11274014/diff/20003/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/11274014/diff/20003/src/heap.h#newcode159
src/heap.h:159: V(JSObject, object_observation_state, ObjectObservationState)
On 2012/11/06 14:54:21, adamk wrote:
> Note, renamed this to just "observation_state" given that the flag changed to
> "harmony_observation". Willing to take "harmony_" on the front if requested.

That's fine. I prefer it the way it is right now as well.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6