Remember and reuse derived map for external arrays

Remember and reuse derived map for external arrays

Ensure that all objects that had the same map before attaching an external array have the same map once the external array is attached.

BUG=75639
TEST=fast/canvas/webgl/uninitialized-test.html

Committed: http://code.google.com/p/v8/source/detail?r=7318

[danno, 2011-03-17 22:39:58]

please review

[danno, 2011-03-17 22:40:24]

please review

[Mads Ager (chromium), 2011-03-21 08:32:40]

We need to deal with setting properties with the empty string as the name on
objects with external array transitions. Other than that it looks good.

http://codereview.chromium.org/6685073/diff/6001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6685073/diff/6001/src/objects.cc#newcode1785
src/objects.cc:1785: String* external_array_sentinel_name =
Heap::empty_string();
The empty string is a valid property name (every string is using the bracket
notation: o[""] = 42). Therefore, we need deal with SetProperty for this type of
transition as well. If there is this type of transitions I guess we can just
drop this transition and replace it with a property instead. I guess that will
mean treating this transition the same a null descriptor in SetProperty. 

We should add a cctest for that case.

http://codereview.chromium.org/6685073/diff/6001/src/objects.cc#newcode1797
src/objects.cc:1797: // If the transition already exists, check the type. If
thee is a match,
thee -> there

[danno, 2011-03-22 16:05:30]

Please take a look again, there were enough changes to warrant a second look.

[Mads Ager (chromium), 2011-03-22 18:57:25]

LGTM

http://codereview.chromium.org/6685073/diff/8001/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/6685073/diff/8001/src/factory.cc#newcode399
src/factory.cc:399: CALL_HEAP_FUNCTION(isolate(),
src->GetExternalArrayElementsMap(
I would prefer a different indentation here. How about:

CALL_HEAP_FUNCTION(isolate(),
                   src->GetExternalArrayElementsMap(array_type,
                                                    safe_to_add_transition),
                   Map);

Or something like that?

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc#newcode1293
src/objects.cc:1293: ASSERT(
Move fist disjunct of the assert to the first line?

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc#newcode1836
src/objects.cc:1836: String* external_array_sentinel_name =
HEAP->empty_symbol();
Let's replace HEAP with GetIsolate()->heap() here to not introduce more uses of
he HEAP macro.

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc#newcode1875
src/objects.cc:1875: COUNTERS->map_to_external_array_elements()->Increment();
GetIsolate()->counters() instead of COUNTERS.

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc#newcode1883
src/objects.cc:1883:
(heap->isolate()->context()->global_context()->object_function()->map() !=
Instead of getting the heap and using heap->isolate() you can do
GetIsolate()->context() here.

http://codereview.chromium.org/6685073/diff/8001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6685073/diff/8001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:11605: result = CompileRun("ext_array[\"\"] = 23;
ext_array[\"\"]");
You can use '' for the empty string in JS code here and below.

[danno, 2011-03-23 09:56:52]

comments addressed and committing

http://codereview.chromium.org/6685073/diff/8001/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/6685073/diff/8001/src/factory.cc#newcode399
src/factory.cc:399: CALL_HEAP_FUNCTION(isolate(),
src->GetExternalArrayElementsMap(
On 2011/03/22 18:57:25, Mads Ager wrote:
> I would prefer a different indentation here. How about:
> 
> CALL_HEAP_FUNCTION(isolate(),
>                    src->GetExternalArrayElementsMap(array_type,
>                                                     safe_to_add_transition),
>                    Map);
> 
> Or something like that?
>  

Done.

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc#newcode1293
src/objects.cc:1293: ASSERT(
On 2011/03/22 18:57:25, Mads Ager wrote:
> Move fist disjunct of the assert to the first line?

Done.

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc#newcode1836
src/objects.cc:1836: String* external_array_sentinel_name =
HEAP->empty_symbol();
On 2011/03/22 18:57:25, Mads Ager wrote:
> Let's replace HEAP with GetIsolate()->heap() here to not introduce more uses
of
> he HEAP macro.

Done.

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc#newcode1875
src/objects.cc:1875: COUNTERS->map_to_external_array_elements()->Increment();
On 2011/03/22 18:57:25, Mads Ager wrote:
> GetIsolate()->counters() instead of COUNTERS.

Done.

http://codereview.chromium.org/6685073/diff/8001/src/objects.cc#newcode1883
src/objects.cc:1883:
(heap->isolate()->context()->global_context()->object_function()->map() !=
On 2011/03/22 18:57:25, Mads Ager wrote:
> Instead of getting the heap and using heap->isolate() you can do
> GetIsolate()->context() here.

Done.

http://codereview.chromium.org/6685073/diff/8001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6685073/diff/8001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:11605: result = CompileRun("ext_array[\"\"] = 23;
ext_array[\"\"]");
On 2011/03/22 18:57:25, Mads Ager wrote:
> You can use '' for the empty string in JS code here and below.

Done.