Some comments: a few real issues and a bunch of nits. These apply to patch
set
6; I haven't looked at patch set 7 yet so maybe some of them are already
obsolete.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements-kind.cc
File src/elements-kind.cc (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements-kind.cc#newcode55
src/elements-kind.cc:55: ElementsKind **fast_elements_kind_sequence_ptr)
{
nit: s/ **/** /
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements-kind.h
File src/elements-kind.h (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements-kind.h#newcode135
src/elements-kind.h:135: return !IsFastHoleyElementsKind(kind);
Let's add an "ASSERT(IsFastElementsKind(kind));" here. Just to prevent
callers from making wrong assumptions.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc
File src/elements.cc (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#newcode502
src/elements.cc:502: virtual MaybeObject* SetCapacityAndLength(
nit: both this and the following change seem unnecessary.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#newcode859
src/elements.cc:859: }
nit: indentation
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#newcode865
src/elements.cc:865: JSObject* obj,
nit: unnecessary formatting change?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#newcode889
src/elements.cc:889:
nit: two empty lines (several more times below)
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#newcode935
src/elements.cc:935: static MaybeObject*
SetFastElementsCapacityAndLength(
nit: yet more formatting changes
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#newcode974
src/elements.cc:974: FastPackedDoubleElementsAccessor,
nit: indentation (same for FastHoleyDoubleElementsAccessor)
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.cc#newcode981
src/elements.cc:981: FastPackedDoubleElementsAccessor,
again
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.h
File src/elements.h (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/elements.h#newcode87
src/elements.h:87: virtual MaybeObject* SetCapacityAndLength(
nit: why this formatting change?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/hydrogen-instructions.h#newcode4370
src/hydrogen-instructions.h:4370: if
(original_map->has_fast_double_elements()) {
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.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/hydrogen.cc
File src/hydrogen.cc (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/hydrogen.cc#newcode5219
src/hydrogen.cc:5219: HInstruction* mapcheck =
AddInstruction(new(zone())
nit: I'd break the line after "AddInstruction("
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):
https://chromiumcodereview.appspot.com/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
The comment says that FAST_*_SMI_ELEMENTS are transitioned, but
FAST_HOLEY_SMI_ELEMENTS actually jump to &slow. Is that intentional?
https://chromiumcodereview.appspot.com/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
nit: s/ONLY_// or s/ONLY_/*_/.
Also, HOLEY elements again aren't handled in the fast path. Intentional?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/macro-assembler-ia32.cc#newcode507
src/ia32/macro-assembler-ia32.cc:507: current_map =
nit: fits on one line?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/macro-assembler-ia32.cc#newcode2188
src/ia32/macro-assembler-ia32.cc:2188: ? FAST_HOLEY_ELEMENTS :
nit: I'd put the : under the ?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/ia32/stub-cache-ia32.cc#newcode1470
src/ia32/stub-cache-ia32.cc:1470: __
LoadTransitionedArrayMapConditional(FAST_HOLEY_SMI_ELEMENTS,
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.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h
File src/objects-inl.h (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h#newcode1343
src/objects-inl.h:1343: global_context->js_array_maps();
nit: fits on one line
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h#newcode1678
src/objects-inl.h:1678: ASSERT(index >= 0 && index < this->length());
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.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h#newcode3954
src/objects-inl.h:3954:
heap->AllocateFixedArrayWithHoles(kElementsKindCount);
Do you mean kFastElementsKindCount here?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects-inl.h#newcode4807
src/objects-inl.h:4807: (IsFastElementsKind(GetElementsKind()) &&
Did you mean IsFastSmiElementsKind?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc
File src/objects.cc (left):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#oldcode9380
src/objects.cc:9380: if (FLAG_trace_elements_transitions) {
I don't see why you would remove this.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc
File src/objects.cc (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newcode900
src/objects.cc:900: // Externalizing twice leaks the external resource,
so its prohibited by the
Why this change? Original version was correct.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newcode1066
src/objects.cc:1066: ? 0 :
nit: I'd put the colon on the next line, directly below the question
mark.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newcode2382
src/objects.cc:2382: // This method is only called when
safe_to_add_transition has been found
nit: s/safe_to_add_transition/safe_to_add/
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newcode2450
src/objects.cc:2450: IsFastObjectElementsKind(to_kind)) {
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.
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newcode9417
src/objects.cc:9417: array_length);
nit: why the line break?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newcode9450
src/objects.cc:9450: ValidateElements();
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?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/objects.cc#newcode9588
src/objects.cc:9588: ?
SetFastDoubleElementsCapacityAndLength(new_length,
nit: why the line break?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc
File src/runtime.cc (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc#newcode4745
src/runtime.cc:4745: FixedDoubleArray* double_array =
Why the line break?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc#newcode4761
src/runtime.cc:4761: FixedArray* object_array =
Why the line break?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc#newcode7021
src/runtime.cc:7021:
RUNTIME_ASSERT(elements_array->HasFastObjectElements() ||
HasFastSmiOrObjectElements()?
https://chromiumcodereview.appspot.com/10170030/diff/9002/src/runtime.cc#newcode9943
src/runtime.cc:9943: JSObject::TransitionElementsKind(array,
FAST_ELEMENTS));
s/FAST_ELEMENTS/to_kind/ !
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/array-literal-transitions.js
File test/mjsunit/array-literal-transitions.js (left):
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/array-literal-transitions.js#oldcode47
test/mjsunit/array-literal-transitions.js:47: function
array_literal_test() {
What happened here? Didn't like this test any more?
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/array-literal-transitions.js
File test/mjsunit/array-literal-transitions.js (right):
https://chromiumcodereview.appspot.com/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.
nit: 2012
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/elements-kind-depends.js
File test/mjsunit/elements-kind-depends.js (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/elements-kind-depends.js#newcode49
test/mjsunit/elements-kind-depends.js:49:
//%OptimizeFunctionOnNextCall(burn);
This doesn't look intentional.
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/elements-transition-hoisting.js
File test/mjsunit/elements-transition-hoisting.js (right):
https://chromiumcodereview.appspot.com/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.
nit: 2012
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/packed-elements.js
File test/mjsunit/packed-elements.js (right):
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/packed-elements.js#newcode28
test/mjsunit/packed-elements.js:28: // Flags: --allow-natives-syntax
--smi-only-arrays
Might want to specify --packed-arrays here too ;-)
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/regress/regress-1849.js
File test/mjsunit/regress/regress-1849.js (right):
https://chromiumcodereview.appspot.com/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.
nit: 2012
https://chromiumcodereview.appspot.com/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
I suspect s/1878/1849/ here.
https://chromiumcodereview.appspot.com/10170030/diff/9002/test/mjsunit/unbox-double-arrays.js
File test/mjsunit/unbox-double-arrays.js (right):
https://chromiumcodereview.appspot.com/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.
nit: 2012
https://chromiumcodereview.appspot.com/10170030/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
