Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(74)

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

Created:
8 years, 2 months ago by adamk
Modified:
8 years, 1 month ago
CC:
v8-dev
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) Patch
M include/v8.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M src/heap.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M src/heap.cc View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download
M src/object-observe.js View 1 2 3 4 5 6 7 8 1 chunk +21 lines, -11 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M src/objects-inl.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M src/runtime.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M src/runtime.cc View 1 2 3 4 5 6 7 8 1 chunk +43 lines, -0 lines 0 comments Download
M test/cctest/cctest.gyp View 1 2 3 1 chunk +1 line, -0 lines 0 comments 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 Download

Messages

Total messages: 11 (0 generated)
adamk
First look at this, no tests yet
8 years, 2 months ago (2012-10-24 15:17:41 UTC) #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 ...
8 years, 2 months ago (2012-10-24 15:21:35 UTC) #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: > > ...
8 years, 2 months ago (2012-10-24 15:50:38 UTC) #3
adamk
+mstarzinger to comment on usage of per-isolate state. He suggested we need to restructure this ...
8 years, 1 month ago (2012-11-05 11:31:35 UTC) #4
Michael Starzinger
Drive-by-comments: As discussed offline with Andreas, I think we will need a separate "InternalWeakMap" that ...
8 years, 1 month ago (2012-11-05 13:08:10 UTC) #5
adamk
Minimal approach: use ObjectHashTable directly from object-observe.js via a few helper runtime functions. Pretty ugly, ...
8 years, 1 month ago (2012-11-05 14:14:48 UTC) #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): ...
8 years, 1 month ago (2012-11-06 12:27:09 UTC) #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 ...
8 years, 1 month ago (2012-11-06 12:59:13 UTC) #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: ...
8 years, 1 month ago (2012-11-06 14:17:44 UTC) #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" ...
8 years, 1 month ago (2012-11-06 14:54:21 UTC) #10
Michael Starzinger
8 years, 1 month ago (2012-11-06 15:04:10 UTC) #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.

Powered by Google App Engine
This is Rietveld 408576698