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

Issue 10909007: Sharing of descriptor arrays. (Closed)

Created:
8 years, 3 months ago by Toon Verwaest
Modified:
8 years, 3 months ago
CC:
v8-dev
Visibility:
Public.

Description

Sharing of descriptor arrays. This CL adds multiple things: Transition arrays do not directly point at their descriptor array anymore, but rather do so via an indirect pointer (a JSGlobalPropertyCell). An ownership bit is added to maps indicating whether it owns its own descriptor array or not. Maps owning a descriptor array can pass on ownership if a transition from that map is generated; but only if the descriptor array stays exactly the same; or if a descriptor is added. Maps that don't have ownership get ownership back if their direct child to which ownership was passed is cleared in ClearNonLiveTransitions. To detect which descriptors in an array are valid, each map knows its own NumberOfOwnDescriptors. Since the descriptors are sorted in order of addition, if we search and find a descriptor with index bigger than this number, it is not valid for the given map. We currently still build up an enumeration cache (although this may disappear). The enumeration cache is always built for the entire descriptor array, even if not all descriptors are owned by the map. Once a descriptor array has an enumeration cache for a given map; this invariant will always be true, even if the descriptor array was extended. The extended array will inherit the enumeration cache from the smaller descriptor array. If a map with more descriptors needs an enumeration cache, it's EnumLength will still be set to invalid, so it will have to recompute the enumeration cache. This new cache will also be valid for smaller maps since they have their own enumlength; and use this to loop over the cache. If the EnumLength is still invalid, but there is already a cache present that is big enough; we just initialize the EnumLength field for the map. When we apply ClearNonLiveTransitions and descriptor ownership is passed back to a parent map, the descriptor array is trimmed in-place and resorted. At the same time, the enumeration cache is trimmed in-place. Only transition arrays contain descriptor arrays. If we transition to a map and pass ownership of the descriptor array along, the child map will not store the descriptor array it owns. Rather its parent will keep the pointer. So for every leaf-map, we find the descriptor array by following the back pointer, reading out the transition array, and fetching the descriptor array from the JSGlobalPropertyCell. If a map has a transition array, we fetch it from there. If a map has undefined as its back-pointer and has no transition array; it is considered to have an empty descriptor array. When we modify properties, we cannot share the descriptor array. To accommodate this, the child map will get its own transition array; even if there are not necessarily any transitions leaving from the child map. This is necessary since it's the only way to store its own descriptor array. Committed: https://code.google.com/p/v8/source/detail?r=12492

Patch Set 1 : u #

Total comments: 18

Patch Set 2 : Addressed jkummerows comments #

Patch Set 3 : Addressed mstarzingers comments #

Patch Set 4 : Implemented x64 #

Patch Set 5 : Shift rather than masking with Smi on x64 #

Patch Set 6 : Ensure descriptor arrays are not shared acrossed severed transition chains. #

Total comments: 4

Patch Set 7 : Ported to ARM. #

Patch Set 8 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+780 lines, -280 lines) Patch
M src/arm/full-codegen-arm.cc View 1 2 3 4 5 6 2 chunks +20 lines, -14 lines 0 comments Download
M src/arm/macro-assembler-arm.h View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 0 comments Download
M src/arm/macro-assembler-arm.cc View 1 2 3 4 5 6 7 1 chunk +19 lines, -2 lines 0 comments Download
M src/bootstrapper.cc View 2 chunks +3 lines, -2 lines 0 comments Download
M src/handles.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/handles.cc View 1 4 chunks +37 lines, -11 lines 0 comments Download
M src/heap.h View 3 chunks +13 lines, -13 lines 0 comments Download
M src/heap.cc View 1 2 3 chunks +6 lines, -3 lines 0 comments Download
M src/ia32/full-codegen-ia32.cc View 2 chunks +16 lines, -8 lines 0 comments Download
M src/ia32/macro-assembler-ia32.h View 1 chunk +5 lines, -4 lines 0 comments Download
M src/ia32/macro-assembler-ia32.cc View 1 2 3 4 5 6 7 1 chunk +19 lines, -2 lines 0 comments Download
M src/mark-compact.cc View 1 2 1 chunk +4 lines, -4 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 12 chunks +47 lines, -25 lines 0 comments Download
M src/objects.cc View 1 2 3 4 5 6 7 36 chunks +349 lines, -92 lines 0 comments Download
M src/objects-debug.cc View 2 chunks +5 lines, -3 lines 0 comments Download
M src/objects-inl.h View 1 2 3 4 5 12 chunks +91 lines, -42 lines 0 comments Download
M src/profile-generator.cc View 2 chunks +25 lines, -10 lines 0 comments Download
M src/property.h View 2 chunks +2 lines, -1 line 0 comments Download
M src/runtime.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/string-stream.cc View 1 chunk +4 lines, -1 line 0 comments Download
M src/transitions.h View 1 2 3 4 5 5 chunks +15 lines, -9 lines 0 comments Download
M src/transitions.cc View 4 chunks +19 lines, -4 lines 0 comments Download
M src/transitions-inl.h View 1 2 3 4 5 2 chunks +15 lines, -11 lines 0 comments Download
M src/x64/full-codegen-x64.cc View 1 2 3 2 chunks +15 lines, -8 lines 0 comments Download
M src/x64/macro-assembler-x64.h View 1 2 3 4 1 chunk +6 lines, -4 lines 0 comments Download
M src/x64/macro-assembler-x64.cc View 1 2 3 4 5 6 7 1 chunk +19 lines, -2 lines 0 comments Download
M test/cctest/test-heap-profiler.cc View 1 chunk +15 lines, -4 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
Toon Verwaest
PTAL. Only ia32 is implemented for now; and the CL includes a temporary fix for ...
8 years, 3 months ago (2012-08-30 14:17:44 UTC) #1
Jakob Kummerow
Just nits. https://chromiumcodereview.appspot.com/10909007/diff/18001/src/handles.cc File src/handles.cc (right): https://chromiumcodereview.appspot.com/10909007/diff/18001/src/handles.cc#newcode794 src/handles.cc:794: if (cache_result) object->map()->SetEnumLength(enum_size); I don't feel strongly ...
8 years, 3 months ago (2012-09-11 14:24:25 UTC) #2
Michael Starzinger
Drive-by LGTM (I just looked at the GC-related stuff). https://chromiumcodereview.appspot.com/10909007/diff/18001/src/heap.cc File src/heap.cc (right): https://chromiumcodereview.appspot.com/10909007/diff/18001/src/heap.cc#newcode2067 src/heap.cc:2067: ...
8 years, 3 months ago (2012-09-11 14:37:05 UTC) #3
Toon Verwaest
Addressed comments. Now porting to x64 and arm. https://chromiumcodereview.appspot.com/10909007/diff/18001/src/handles.cc File src/handles.cc (right): https://chromiumcodereview.appspot.com/10909007/diff/18001/src/handles.cc#newcode794 src/handles.cc:794: if ...
8 years, 3 months ago (2012-09-11 14:49:23 UTC) #4
Jakob Kummerow
LGTM with nits. https://chromiumcodereview.appspot.com/10909007/diff/18002/src/objects.cc File src/objects.cc (right): https://chromiumcodereview.appspot.com/10909007/diff/18002/src/objects.cc#newcode7443 src/objects.cc:7443: target->instance_descriptors() == descriptors) { nit: can ...
8 years, 3 months ago (2012-09-12 16:31:03 UTC) #5
Toon Verwaest
8 years, 3 months ago (2012-09-12 16:34:59 UTC) #6
Addressed comments

https://chromiumcodereview.appspot.com/10909007/diff/18002/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10909007/diff/18002/src/objects.cc#new...
src/objects.cc:7443: target->instance_descriptors() == descriptors) {
On 2012/09/12 16:31:03, Jakob wrote:
> nit: can use target_descriptors here

Done.

https://chromiumcodereview.appspot.com/10909007/diff/18002/src/x64/macro-asse...
File src/x64/macro-assembler-x64.cc (right):

https://chromiumcodereview.appspot.com/10909007/diff/18002/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:2931: bind(&fail);
On 2012/09/12 16:31:03, Jakob wrote:
> nit: please swap this line and the empty line below

Done.

Powered by Google App Engine
This is Rietveld 408576698