I've taken a first look at elements-kind.h, elements.{h,cc}, objects.cc. The
general approach looks good.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; nit: a method named IsFast... returning true for DICTIONARY_ELEMENTS is somewhat surprising. http://codereview.chromium.org/9839007/diff/5001/src/elements-kind.h#newcode132 src/elements-kind.h:132: inline bool IsFastPackedElementsKind(ElementsKind kind) { 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. http://codereview.chromium.org/9839007/diff/5001/src/elements-kind.h#newcode188 src/elements-kind.h:188: return to_kind != FAST_SMI_ONLY_ELEMENTS && nit: shorter version: return to_kind == FAST_ELEMENTS || to_kind == FAST_HOLEY_ELEMENTS; 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, nit: indentation (also several times below) http://codereview.chromium.org/9839007/diff/5001/src/elements.cc#newcode925 src/elements.cc:925: FastPackedDoubleElementsAccessor, nit: indentation (also below) http://codereview.chromium.org/9839007/diff/5001/src/elements.cc#newcode1344 src/elements.cc:1344: return FastHoleyObjectElementsAccessor::DeleteCommon(obj, key, mode); IIUC the method actually lives in FastElementsAccessor. 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. nit: s/it's/its/ http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode2126 src/objects.cc:2126: switch (this->map()->elements_kind()) { Shouldn't this be implemented in terms of ElementsAccessors? http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode2147 src/objects.cc:2147: if (length != 0) { nit: You don't need the "if", the loop's "i < length" condition is enough (or are you expecting negative lengths?). 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 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." 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 nit: s/the transition/transitions/ (or s/are/is/ two lines down) 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 nit: s/require // http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9300 src/objects.cc:9300: bool introduces_holes = true; IIUC, this lets non-JSArray objects always go into HOLEY mode. Is that intentional? http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9328 src/objects.cc:9328: if (HasFastSmiElements() && !value->IsSmi() && value->IsNumber()) { 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;" http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9577 src/objects.cc:9577: bool introduces_holes = true; Same question here: is it intentional that non-array objects always go HOLEY? http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9580 src/objects.cc:9580: introduces_holes = index > length; I think this comparison must be done *after* the call to JSArray::cast(this)->length()->ToArrayIndex(&length). http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9595 src/objects.cc:9595: if (!maybe_obj->ToObject(&obj)) return maybe_obj; If you rewrite this as "if (maybe_obj->IsFailure()) ...", you don't need "obj" anymore. http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9638 src/objects.cc:9638: if (!maybe_obj->ToObject(&obj)) return maybe_obj; If you rewrite this as "if (maybe_obj->IsFailure()) ...", you don't need "obj" anymore. http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode9960 src/objects.cc:9960: // And HOLEY -> PACKED not allowed. nit: sentence missing its verb. http://codereview.chromium.org/9839007/diff/5001/src/objects.cc#newcode13225 src/objects.cc:13225: nit: intentional? http://codereview.chromium.org/9839007/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
