Reviewers: Jakob,
Message:
Feedback addressed, but moved into another CL since the upload of one of the
files didn't work in patch set 4.
http://codereview.chromium.org/9839007/diff/5001/src/elements-kind.h
File src/elements-kind.h (right):
http://codereview.chromium.org/9839007/diff/5001/src/elements-kind.h#newcode128
src/elements-kind.h:128: kind == DICTIONARY_ELEMENTS;
On 2012/04/23 13:54:28, Jakob wrote:
nit: a method named IsFast... returning true for DICTIONARY_ELEMENTS
is somewhat
surprising.
Done.
http://codereview.chromium.org/9839007/diff/5001/src/elements-kind.h#newcode132
src/elements-kind.h:132: inline bool
IsFastPackedElementsKind(ElementsKind kind) {
On 2012/04/23 13:54:28, Jakob wrote:
Do we need both IsFastHoley and IsFastPacked? Isn't ∀x:
IsFastElementsKind(x) ⇒
IsFastHoley(x) == !IsFastPacked(x)? You could implement one in terms
of the
other, or ASSERT that this assumption is true.
Done.
http://codereview.chromium.org/9839007/diff/5001/src/elements-kind.h#newcode188
src/elements-kind.h:188: return to_kind != FAST_SMI_ONLY_ELEMENTS &&
On 2012/04/23 13:54:28, Jakob wrote:
nit: shorter version:
return to_kind == FAST_ELEMENTS ||
to_kind == FAST_HOLEY_ELEMENTS;
Done.
http://codereview.chromium.org/9839007/diff/5001/src/elements.cc
File src/elements.cc (right):
http://codereview.chromium.org/9839007/diff/5001/src/elements.cc#newcode835
src/elements.cc:835: FastPackedSmiElementsAccessor,
On 2012/04/23 13:54:28, Jakob wrote:
nit: indentation (also several times below)
Done.
http://codereview.chromium.org/9839007/diff/5001/src/elements.cc#newcode925
src/elements.cc:925: FastPackedDoubleElementsAccessor,
On 2012/04/23 13:54:28, Jakob wrote:
nit: indentation (also below)
Done.
http://codereview.chromium.org/9839007/diff/5001/src/elements.cc#newcode1344
src/elements.cc:1344: return
FastHoleyObjectElementsAccessor::DeleteCommon(obj, key, mode);
On 2012/04/23 13:54:28, Jakob wrote:
IIUC the method actually lives in FastElementsAccessor.
The template magic makes this particularly hard to do. Before I refactor
the routine outside the class, I will just add an appropriate comment
here.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode2108
src/objects.cc:2108: // When objects are first allocated, it's elements
are Failures.
On 2012/04/23 13:54:28, Jakob wrote:
nit: s/it's/its/
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode2126
src/objects.cc:2126: switch (this->map()->elements_kind()) {
On 2012/04/23 13:54:28, Jakob wrote:
Shouldn't this be implemented in terms of ElementsAccessors?
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode2147
src/objects.cc:2147: if (length != 0) {
On 2012/04/23 13:54:28, Jakob wrote:
nit: You don't need the "if", the loop's "i < length" condition is
enough (or
are you expecting negative lengths?).
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode2461
src/objects.cc:2461: // If the transition is a single step in the
lattice, add the transition to
On 2012/04/23 13:54:28, Jakob wrote:
This comment doesn't quite make sense. Suggestion (also eliminates the
need for
the comment right inside the if-block):
"If the transition is a single step in the transition sequence, fall
through to
looking it up and returning it. If it requires several steps, divide
and
conquer."
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode2493
src/objects.cc:2493: // The map transition graph should be a tree,
therefore the transition to
On 2012/04/23 13:54:28, Jakob wrote:
nit: s/the transition/transitions/ (or s/are/is/ two lines down)
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode2494
src/objects.cc:2494: // ElementsKind that require are not adjacent in
the ElementsKind sequence
On 2012/04/23 13:54:28, Jakob wrote:
nit: s/require //
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9300
src/objects.cc:9300: bool introduces_holes = true;
On 2012/04/23 13:54:28, Jakob wrote:
IIUC, this lets non-JSArray objects always go into HOLEY mode. Is that
intentional?
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9328
src/objects.cc:9328: if (HasFastSmiElements() && !value->IsSmi() &&
value->IsNumber()) {
On 2012/04/23 13:54:28, Jakob wrote:
The ElementsKind checks/assignments in this method deserve some
cleanup.
- Consider moving "ElementsKind elements_kind = GetElementsKind()" all
the way
up to the top, because you need it everywhere.
- Consider moving "bool can_pack =
IsFastPackedElementsKind(elements_kind) &&
!introduces_holes" to the top, too, because you need it almost
everywhere.
- In cases where you know what elements you'll end up with, consider
assigning
them directly instead of calling helper functions, e.g. for SMI->FAST
transitions: "ElementsKind new_kind = can_pack ? FAST_ELEMENTS :
FAST_HOLEY_ELEMENTS;"
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9577
src/objects.cc:9577: bool introduces_holes = true;
On 2012/04/23 13:54:28, Jakob wrote:
Same question here: is it intentional that non-array objects always go
HOLEY?
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9580
src/objects.cc:9580: introduces_holes = index > length;
On 2012/04/23 13:54:28, Jakob wrote:
I think this comparison must be done *after* the call to
JSArray::cast(this)->length()->ToArrayIndex(&length).
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9595
src/objects.cc:9595: if (!maybe_obj->ToObject(&obj)) return maybe_obj;
On 2012/04/23 13:54:28, Jakob wrote:
If you rewrite this as "if (maybe_obj->IsFailure()) ...", you don't
need "obj"
anymore.
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9638
src/objects.cc:9638: if (!maybe_obj->ToObject(&obj)) return maybe_obj;
On 2012/04/23 13:54:28, Jakob wrote:
If you rewrite this as "if (maybe_obj->IsFailure()) ...", you don't
need "obj"
anymore.
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9960
src/objects.cc:9960: // And HOLEY -> PACKED not allowed.
On 2012/04/23 13:54:28, Jakob wrote:
nit: sentence missing its verb.
Done.
http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode13225
src/objects.cc:13225:
On 2012/04/23 13:54:28, Jakob wrote:
nit: intentional?
Done.
Description:
Implement PACKED and HOLEY ElementsKinds.
Please review this at http://codereview.chromium.org/9839007/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/api.h
M src/api.cc
M src/arm/code-stubs-arm.cc
M src/arm/codegen-arm.cc
M src/arm/full-codegen-arm.cc
M src/arm/ic-arm.cc
M src/arm/lithium-codegen-arm.cc
M src/arm/stub-cache-arm.cc
M src/bootstrapper.cc
M src/builtins.cc
M src/checks.cc
M src/code-stubs.cc
M src/codegen.h
M src/contexts.h
A src/elements-kind.h
M src/elements.h
M src/elements.cc
M src/factory.h
M src/factory.cc
M src/flag-definitions.h
M src/heap.cc
M src/hydrogen-instructions.h
M src/hydrogen-instructions.cc
M src/hydrogen.cc
M src/ia32/builtins-ia32.cc
M src/ia32/code-stubs-ia32.cc
M src/ia32/codegen-ia32.cc
M src/ia32/full-codegen-ia32.cc
M src/ia32/ic-ia32.cc
M src/ia32/lithium-codegen-ia32.cc
M src/ia32/lithium-ia32.cc
M src/ia32/macro-assembler-ia32.h
M src/ia32/macro-assembler-ia32.cc
M src/ia32/stub-cache-ia32.cc
M src/ic.h
M src/ic.cc
M src/jsregexp.cc
M src/lithium.cc
M src/mips/code-stubs-mips.cc
M src/mips/codegen-mips.cc
M src/mips/ic-mips.cc
M src/mips/lithium-codegen-mips.cc
M src/mips/lithium-mips.cc
M src/mips/stub-cache-mips.cc
M src/objects-debug.cc
M src/objects-inl.h
M src/objects-printer.cc
M src/objects.h
M src/objects.cc
M src/parser.cc
M src/profile-generator.cc
M src/runtime.h
M src/runtime.cc
M src/string-stream.cc
M src/x64/code-stubs-x64.cc
M src/x64/codegen-x64.cc
M src/x64/ic-x64.cc
M src/x64/stub-cache-x64.cc
M test/cctest/test-heap.cc
M test/mjsunit/array-construct-transition.js
M test/mjsunit/array-literal-transitions.js
M test/mjsunit/elements-kind-depends.js
M test/mjsunit/elements-kind.js
M test/mjsunit/elements-transition-hoisting.js
M test/mjsunit/elements-transition.js
A test/mjsunit/holey-elements.js
A + test/mjsunit/packed-elements.js
M test/mjsunit/regress/regress-1849.js
test/mjsunit/regress/regress-1878.js
M test/mjsunit/regress/regress-crbug-122271.js
M test/mjsunit/regress/regress-smi-only-concat.js
M test/mjsunit/unbox-double-arrays.js
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
