Please take another look.
http://codereview.chromium.org/10170030/diff/9002/src/elements-kind.cc
File src/elements-kind.cc (right):
http://codereview.chromium.org/10170030/diff/9002/src/elements-kind.cc#newcode55
src/elements-kind.cc:55: ElementsKind **fast_elements_kind_sequence_ptr)
{
On 2012/05/13 21:55:27, Jakob wrote:
nit: s/ **/** /
Done.
http://codereview.chromium.org/10170030/diff/9002/src/elements-kind.h
File src/elements-kind.h (right):
http://codereview.chromium.org/10170030/diff/9002/src/elements-kind.h#newcode135
src/elements-kind.h:135: return !IsFastHoleyElementsKind(kind);
On 2012/05/13 21:55:27, Jakob wrote:
Let's add an "ASSERT(IsFastElementsKind(kind));" here. Just to prevent
callers
from making wrong assumptions.
Done.
http://codereview.chromium.org/10170030/diff/9002/src/elements.cc
File src/elements.cc (right):
http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode502
src/elements.cc:502: virtual MaybeObject* SetCapacityAndLength(
On 2012/05/13 21:55:27, Jakob wrote:
nit: both this and the following change seem unnecessary.
Done.
http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode859
src/elements.cc:859: }
On 2012/05/13 21:55:27, Jakob wrote:
nit: indentation
Done.
http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode865
src/elements.cc:865: JSObject* obj,
On 2012/05/13 21:55:27, Jakob wrote:
nit: unnecessary formatting change?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode889
src/elements.cc:889:
On 2012/05/13 21:55:27, Jakob wrote:
nit: two empty lines (several more times below)
Done.
http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode935
src/elements.cc:935: static MaybeObject*
SetFastElementsCapacityAndLength(
On 2012/05/13 21:55:27, Jakob wrote:
nit: yet more formatting changes
Done.
http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode974
src/elements.cc:974: FastPackedDoubleElementsAccessor,
On 2012/05/13 21:55:27, Jakob wrote:
nit: indentation (same for FastHoleyDoubleElementsAccessor)
Done.
http://codereview.chromium.org/10170030/diff/9002/src/elements.cc#newcode981
src/elements.cc:981: FastPackedDoubleElementsAccessor,
On 2012/05/13 21:55:27, Jakob wrote:
again
Done.
http://codereview.chromium.org/10170030/diff/9002/src/elements.h
File src/elements.h (right):
http://codereview.chromium.org/10170030/diff/9002/src/elements.h#newcode87
src/elements.h:87: virtual MaybeObject* SetCapacityAndLength(
On 2012/05/13 21:55:27, Jakob wrote:
nit: why this formatting change?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
http://codereview.chromium.org/10170030/diff/9002/src/hydrogen-instructions.h#newcode4370
src/hydrogen-instructions.h:4370: if
(original_map->has_fast_double_elements()) {
On 2012/05/13 21:55:27, Jakob wrote:
The bodies of these two if-blocks are the same; I'd suggest to merge
them. IIUC,
the most appropriate condition is "if
(original_map->has_fast_double_elements()
^ transitioned_map->has_fast_double_elements())", as a PACKED_DOUBLE
->
HOLEY_DOUBLE transition doesn't need to set those flags.
Done.
http://codereview.chromium.org/10170030/diff/9002/src/hydrogen.cc
File src/hydrogen.cc (right):
http://codereview.chromium.org/10170030/diff/9002/src/hydrogen.cc#newcode5219
src/hydrogen.cc:5219: HInstruction* mapcheck =
AddInstruction(new(zone())
On 2012/05/13 21:55:27, Jakob wrote:
nit: I'd break the line after "AddInstruction("
Done.
http://codereview.chromium.org/10170030/diff/9002/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):
http://codereview.chromium.org/10170030/diff/9002/src/ia32/ic-ia32.cc#newcode892
src/ia32/ic-ia32.cc:892: // Value is a double. Transition
FAST_*_SMI_ELEMENTS -> FAST_*_DOUBLE_ELEMENTS
For now, yes. When Toon finishes his changes, all cases will be handled
here.
On 2012/05/13 21:55:27, Jakob wrote:
The comment says that FAST_*_SMI_ELEMENTS are transitioned, but
FAST_HOLEY_SMI_ELEMENTS actually jump to &slow. Is that intentional?
http://codereview.chromium.org/10170030/diff/9002/src/ia32/ic-ia32.cc#newcode904
src/ia32/ic-ia32.cc:904: // Value is not a double,
FAST_SMI_ONLY_ELEMENTS -> FAST_ELEMENTS
On 2012/05/13 21:55:27, Jakob wrote:
nit: s/ONLY_// or s/ONLY_/*_/.
Also, HOLEY elements again aren't handled in the fast path.
Intentional?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):
http://codereview.chromium.org/10170030/diff/9002/src/ia32/macro-assembler-ia32.cc#newcode507
src/ia32/macro-assembler-ia32.cc:507: current_map =
On 2012/05/13 21:55:27, Jakob wrote:
nit: fits on one line?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/ia32/macro-assembler-ia32.cc#newcode2188
src/ia32/macro-assembler-ia32.cc:2188: ? FAST_HOLEY_ELEMENTS :
On 2012/05/13 21:55:27, Jakob wrote:
nit: I'd put the : under the ?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/10170030/diff/9002/src/ia32/stub-cache-ia32.cc#newcode1470
src/ia32/stub-cache-ia32.cc:1470: __
LoadTransitionedArrayMapConditional(FAST_HOLEY_SMI_ELEMENTS,
On 2012/05/13 21:55:27, Jakob wrote:
Didn't you mean to check for packed FAST_SMI_ELEMENTS here, and then
for
FAST_HOLEY_SMI_ELEMENTS after "__ bind(&try_holey_map)"? I guess it
doesn't
really make a difference, but the current naming is confusing.
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects-inl.h
File src/objects-inl.h (right):
http://codereview.chromium.org/10170030/diff/9002/src/objects-inl.h#newcode1343
src/objects-inl.h:1343: global_context->js_array_maps();
On 2012/05/13 21:55:27, Jakob wrote:
nit: fits on one line
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects-inl.h#newcode1678
src/objects-inl.h:1678: ASSERT(index >= 0 && index < this->length());
On 2012/05/13 21:55:27, Jakob wrote:
nit: this ASSERT, while correct, is not necessary, as it's covered by
the
"get(index)" call anyway. Feel free to leave it in if you prefer,
though.
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects-inl.h#newcode3954
src/objects-inl.h:3954:
heap->AllocateFixedArrayWithHoles(kElementsKindCount);
On 2012/05/13 21:55:27, Jakob wrote:
Do you mean kFastElementsKindCount here?
kElementsKindCount is better here, since it removes the need to
implement special cases when testing against DICTIONARY and other maps.
In this array, they are initialize with holes, so comparisons always
fail without having to gate code elsewhere with IsFastElementKind()
checks before looking into the map array.
http://codereview.chromium.org/10170030/diff/9002/src/objects-inl.h#newcode4807
src/objects-inl.h:4807: (IsFastElementsKind(GetElementsKind()) &&
On 2012/05/13 21:55:27, Jakob wrote:
Did you mean IsFastSmiElementsKind?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc
File src/objects.cc (left):
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#oldcode9380
src/objects.cc:9380: if (FLAG_trace_elements_transitions) {
Because it's a dupe of the transition printing done in
SetFastElementsCapacityAndLength if it actually changes ElementsKind.
On 2012/05/13 21:55:27, Jakob wrote:
I don't see why you would remove this.
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode900
src/objects.cc:900: // Externalizing twice leaks the external resource,
so its prohibited by the
On 2012/05/13 21:55:27, Jakob wrote:
Why this change? Original version was correct.
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode1066
src/objects.cc:1066: ? 0 :
On 2012/05/13 21:55:27, Jakob wrote:
nit: I'd put the colon on the next line, directly below the question
mark.
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode2382
src/objects.cc:2382: // This method is only called when
safe_to_add_transition has been found
On 2012/05/13 21:55:27, Jakob wrote:
nit: s/safe_to_add_transition/safe_to_add/
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode2450
src/objects.cc:2450: IsFastObjectElementsKind(to_kind)) {
On 2012/05/13 21:55:27, Jakob wrote:
Wouldn't it make sense to check for any FAST kind here? Not sure if it
would
ever happen in practice, but we don't want long chains of e.g.
DICTIONARY <->
FAST_SMI maps either.
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode9417
src/objects.cc:9417: array_length);
On 2012/05/13 21:55:27, Jakob wrote:
nit: why the line break?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode9450
src/objects.cc:9450: ValidateElements();
On 2012/05/13 21:55:27, Jakob wrote:
Calls to ValidateElements() aren't wrapped in an #if DEBUG elsewhere
(e.g. in
line 9420). Aren't they essentially no-ops in non-DEBUG mode anyway?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/objects.cc#newcode9588
src/objects.cc:9588: ?
SetFastDoubleElementsCapacityAndLength(new_length,
On 2012/05/13 21:55:27, Jakob wrote:
nit: why the line break?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc#newcode4745
src/runtime.cc:4745: FixedDoubleArray* double_array =
On 2012/05/13 21:55:27, Jakob wrote:
Why the line break?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc#newcode4761
src/runtime.cc:4761: FixedArray* object_array =
On 2012/05/13 21:55:27, Jakob wrote:
Why the line break?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc#newcode7021
src/runtime.cc:7021:
RUNTIME_ASSERT(elements_array->HasFastObjectElements() ||
On 2012/05/13 21:55:27, Jakob wrote:
HasFastSmiOrObjectElements()?
Done.
http://codereview.chromium.org/10170030/diff/9002/src/runtime.cc#newcode9943
src/runtime.cc:9943: JSObject::TransitionElementsKind(array,
FAST_ELEMENTS));
On 2012/05/13 21:55:27, Jakob wrote:
s/FAST_ELEMENTS/to_kind/ !
Done.
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/array-literal-transitions.js
File test/mjsunit/array-literal-transitions.js (left):
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/array-literal-transitions.js#oldcode47
test/mjsunit/array-literal-transitions.js:47: function
array_literal_test() {
On 2012/05/13 21:55:27, Jakob wrote:
What happened here? Didn't like this test any more?
Done.
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/array-literal-transitions.js
File test/mjsunit/array-literal-transitions.js (right):
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/array-literal-transitions.js#newcode1
test/mjsunit/array-literal-transitions.js:1: // Copyright 2011 the V8
project authors. All rights reserved.
On 2012/05/13 21:55:27, Jakob wrote:
nit: 2012
Done.
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/elements-kind-depends.js
File test/mjsunit/elements-kind-depends.js (right):
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/elements-kind-depends.js#newcode49
test/mjsunit/elements-kind-depends.js:49:
//%OptimizeFunctionOnNextCall(burn);
On 2012/05/13 21:55:27, Jakob wrote:
This doesn't look intentional.
Done.
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/elements-transition-hoisting.js
File test/mjsunit/elements-transition-hoisting.js (right):
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/elements-transition-hoisting.js#newcode1
test/mjsunit/elements-transition-hoisting.js:1: // Copyright 2011 the V8
project authors. All rights reserved.
On 2012/05/13 21:55:27, Jakob wrote:
nit: 2012
Done.
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/packed-elements.js
File test/mjsunit/packed-elements.js (right):
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/packed-elements.js#newcode28
test/mjsunit/packed-elements.js:28: // Flags: --allow-natives-syntax
--smi-only-arrays
On 2012/05/13 21:55:27, Jakob wrote:
Might want to specify --packed-arrays here too ;-)
Done.
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/regress/regress-1849.js
File test/mjsunit/regress/regress-1849.js (right):
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/regress/regress-1849.js#newcode1
test/mjsunit/regress/regress-1849.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
On 2012/05/13 21:55:27, Jakob wrote:
nit: 2012
Done.
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/regress/regress-1849.js#newcode28
test/mjsunit/regress/regress-1849.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=1878
On 2012/05/13 21:55:27, Jakob wrote:
I suspect s/1878/1849/ here.
Done.
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/unbox-double-arrays.js
File test/mjsunit/unbox-double-arrays.js (right):
http://codereview.chromium.org/10170030/diff/9002/test/mjsunit/unbox-double-arrays.js#newcode1
test/mjsunit/unbox-double-arrays.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
On 2012/05/13 21:55:27, Jakob wrote:
nit: 2012
Done.
http://codereview.chromium.org/10170030/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
