I'm not convinced this is the way to go to make sorting faster. I'm not
sure I
like the "prepare for multiple writes" approach, although I can see the
advantage of not updating the write barrier for every step one takes during
a
permutation. It just seems ... fragile.
In the easy cases (no element accessors, no inherited elements, etc.),
maybe we
should go to pure C++ code, and then leave a JavaScript version for the
cases
with complications (which never happens in practice).
The only problem with that approach is calling the compare function will
have us
jumping in and out of JS all the time.
http://codereview.chromium.org/1737007/diff/2001/3001
File src/array.js (right):
http://codereview.chromium.org/1737007/diff/2001/3001#newcode683
src/array.js:683: %_SwapElements(a, i, low_end);
It would seem this does one read more than the current code (a value
that we already have in a variable).
Can't that be helped? Or is it faster anyway?
http://codereview.chromium.org/1737007/diff/2001/3003
File src/heap-inl.h (right):
http://codereview.chromium.org/1737007/diff/2001/3003#newcode184
src/heap-inl.h:184: additional_scavenge_roots_.Add(object);
Why not just use a handle?
http://codereview.chromium.org/1737007/diff/2001/3006
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/1737007/diff/2001/3006#newcode6622
src/ia32/codegen-ia32.cc:6622: __ CallRuntime(Runtime::kSwapElements,
3);
So if the JSArray have index-accessors, we go to runtime for every Swap
(twice per loop iteration of the sort function).
Wouldn't it be better to have two sort versions - one in pure
JavaScript, and one optimized version for when we are sorting a plain
Array with no complications. Then we just need to test for complications
once.
http://codereview.chromium.org/1737007/diff/2001/3006#newcode6630
src/ia32/codegen-ia32.cc:6630: // Note: object MUST have been prepared
for multiple writes!
Which object? Where do you test it?
http://codereview.chromium.org/1737007/diff/2001/3006#newcode6661
src/ia32/codegen-ia32.cc:6661: // Check the object's elements are in
fast case.
You need to check that object is a JSObject.
This is a runtime-function, you must check arguments throughly.
http://codereview.chromium.org/1737007/diff/2001/3006#newcode6672
src/ia32/codegen-ia32.cc:6672: if (FLAG_debug_code) __
AbortIfNotSmi(index2.reg());
These tests should always be made. Don't trust parameters.
http://codereview.chromium.org/1737007/diff/2001/3006#newcode6693
src/ia32/codegen-ia32.cc:6693: FixedArray::kHeaderSize), object.reg());
Swap the stores, so the value read first is also written first (I guess
it will give it better time to resolve the load - even if it's writing
to the element that is currently being read).
http://codereview.chromium.org/1737007/diff/2001/3006#newcode6695
src/ia32/codegen-ia32.cc:6695: // for multiple writes before.
You need to be able to test that. This code must not fail if a
non-prepared object gets here.
http://codereview.chromium.org/1737007/diff/2001/3009
File src/runtime.cc (right):
http://codereview.chromium.org/1737007/diff/2001/3009#newcode7769
src/runtime.cc:7769: // we properly updated remembered set on each swap.
What if the array changed to non-fast mode *during* sorting?
Remember we use this with a user-provided comparison function as well,
so it might change any assumption at any time during sorting. Including
adding accessors and changing the length of the array.
http://codereview.chromium.org/1737007/diff/2001/3009#newcode7785
src/runtime.cc:7785: Handle<Object> object = args.at<Object>(0);
Use
CONVERT_ARG_CHECKED(JSObject, object, 0);
That also throws the appropriate exception if conversion fails.
This is not so important if this function is never called directly from
JS code, but then you need to check the arguments in the _SwapElements
code.
http://codereview.chromium.org/1737007/diff/2001/3009#newcode7789
src/runtime.cc:7789: Handle<Object> tmp1 = GetProperty(object, key1);
Shouldn't this be GetElement? Ditto for SetElement below.
http://codereview.chromium.org/1737007/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
