Skip to content

Commit c565b12

Browse files
committed
[MERGE #5724 @rhuanjl] Implement stable MergeSort for Array.prototype.sort
Merge pull request #5724 from rhuanjl:mergeSort Picking up on the discussion in #5661 This PR implements a stable bottom up Merge Sort as a JsBuiltin for arrays of any length up to 2^32 (well I hit out of memory trying to allocate an array with length above 2^29 but in theory). I'm not sure if it's good enough to merge as is but would appreciate feedback. **EDIT:** I've made some large edits to the below to reflect changes made. **Issues to consider:** 1. **Performance - DefaultCompare** - My Default Compare sort is very slow despite cacheing all the string conversions at the start it. The string less than operation is a significant bottle neck - I have tried: - a native chakraLibrary method to compare strings - this was about the same performance as using less than - using charCodeAt in a loop - this was also about the same performance as using less than 1. **Insertion sort** - I have included an insertion sort directly in the Array.prototype.sort function used for short arrays - could consider what the best cut off is before switching to mergeSort instead - currently length of 2048 is used. 1. **Memory usage** - My implementation of merge sort needs a buffer array with length up to half the length of the array being sorted. 1. **Scope** - I've not looked at the sort method for TypedArrays obviously stabilising that doesn't make sense (though may be worth looking at its performance on xplat as it uses the earlier mentioned slow xplat qsort for arrays of any length) 1. **Tests** - I've consolidated most of the pre-existing array sort tests and also added a test for sorting a variety of random arrays and ensuring that the sort is both correct and stable 1. **General** - see other comments I've added below... fixes: #5661 fixes: #5719
2 parents 4d6cf88 + 94b481d commit c565b12

23 files changed

+9200
-7334
lines changed

lib/Runtime/Base/JnDirectFields.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,7 @@ ENTRY(Array_values)
650650
ENTRY(Array_keys)
651651
ENTRY(Array_entries)
652652
ENTRY(Array_indexOf)
653+
ENTRY(Array_sort)
653654
ENTRY(Array_filter)
654655
ENTRY(Array_flat)
655656
ENTRY(Array_flatMap)

lib/Runtime/ByteCode/ByteCodeCacheReleaseFileVersion.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,6 @@
44
//-------------------------------------------------------------------------------------------------------
55
// NOTE: If there is a merge conflict the correct fix is to make a new GUID.
66

7-
// {D8913E7E-E430-4B28-81DD-EDD3EE5F263B}
7+
// {49A3F597-0DAD-4BD8-82A0-CEAC52C99E63}
88
const GUID byteCodeCacheReleaseFileVersion =
9-
{ 0xD8913E7E, 0xE430, 0x4B28, { 0x81, 0xDD, 0xED, 0xD3, 0xEE, 0x5F, 0x26, 0x3B } };
9+
{ 0x49A3F597, 0x0DAD, 0x4BD8, { 0x82, 0xA0, 0xCE, 0xAC, 0x52, 0xC9, 0x9E, 0x63 } };

lib/Runtime/Library/InJavascript/Intl.js.bc.32b.h

Lines changed: 785 additions & 785 deletions
Large diffs are not rendered by default.

lib/Runtime/Library/InJavascript/Intl.js.bc.64b.h

Lines changed: 788 additions & 788 deletions
Large diffs are not rendered by default.

lib/Runtime/Library/InJavascript/Intl.js.nojit.bc.32b.h

Lines changed: 793 additions & 793 deletions
Large diffs are not rendered by default.

lib/Runtime/Library/InJavascript/Intl.js.nojit.bc.64b.h

Lines changed: 772 additions & 772 deletions
Large diffs are not rendered by default.

lib/Runtime/Library/JavascriptLibrary.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1796,7 +1796,6 @@ namespace Js
17961796
builtinFuncs[BuiltinFunction::JavascriptArray_Reverse] = library->AddFunctionToLibraryObject(arrayPrototype, PropertyIds::reverse, &JavascriptArray::EntryInfo::Reverse, 0);
17971797
builtinFuncs[BuiltinFunction::JavascriptArray_Shift] = library->AddFunctionToLibraryObject(arrayPrototype, PropertyIds::shift, &JavascriptArray::EntryInfo::Shift, 0);
17981798
builtinFuncs[BuiltinFunction::JavascriptArray_Slice] = library->AddFunctionToLibraryObject(arrayPrototype, PropertyIds::slice, &JavascriptArray::EntryInfo::Slice, 2);
1799-
/* No inlining Array_Sort */ library->AddFunctionToLibraryObject(arrayPrototype, PropertyIds::sort, &JavascriptArray::EntryInfo::Sort, 1);
18001799
builtinFuncs[BuiltinFunction::JavascriptArray_Splice] = library->AddFunctionToLibraryObject(arrayPrototype, PropertyIds::splice, &JavascriptArray::EntryInfo::Splice, 2);
18011800

18021801
// The toString and toLocaleString properties are shared between Array.prototype and %TypedArray%.prototype.
@@ -1822,6 +1821,7 @@ namespace Js
18221821
{
18231822
builtinFuncs[BuiltinFunction::JavascriptArray_IndexOf] = library->AddFunctionToLibraryObject(arrayPrototype, PropertyIds::indexOf, &JavascriptArray::EntryInfo::IndexOf, 1);
18241823
builtinFuncs[BuiltinFunction::JavascriptArray_Includes] = library->AddFunctionToLibraryObject(arrayPrototype, PropertyIds::includes, &JavascriptArray::EntryInfo::Includes, 1);
1824+
/* No inlining Array_Sort */ library->AddFunctionToLibraryObject(arrayPrototype, PropertyIds::sort, &JavascriptArray::EntryInfo::Sort, 1);
18251825
}
18261826

18271827
builtinFuncs[BuiltinFunction::JavascriptArray_LastIndexOf] = library->AddFunctionToLibraryObject(arrayPrototype, PropertyIds::lastIndexOf, &JavascriptArray::EntryInfo::LastIndexOf, 1);

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,205 @@
174174
}
175175
});
176176

177+
platform.registerChakraLibraryFunction("MergeSort", function(array, length, compareFn) {
178+
const buffer = [];
179+
buffer.__proto__ = null;
180+
181+
let bucketSize = 2, lastSize = 1, position = 0;
182+
const doubleLength = length + length;
183+
184+
while (bucketSize < doubleLength) {
185+
while (position < length) {
186+
const left = position;
187+
const mid = left + lastSize;
188+
189+
// perform a merge but only if it's necessary
190+
if (mid < length && compareFn(array[mid], array[mid - 1]) < 0) {
191+
let right = position + bucketSize;
192+
right = right < length ? right : length;
193+
let i = mid - 1, j = 0, k = mid;
194+
195+
while (k < right) {
196+
buffer[j++] = array[k++];
197+
}
198+
199+
let rightElement = buffer[--j];
200+
let leftElement = array[i];
201+
202+
for (;;) {
203+
if (compareFn(rightElement, leftElement) < 0) {
204+
array[--k] = leftElement;
205+
if (i > left) {
206+
leftElement = array[--i];
207+
} else {
208+
array[--k] = rightElement;
209+
break;
210+
}
211+
} else {
212+
array[--k] = rightElement;
213+
if (j > 0) {
214+
rightElement = buffer[--j];
215+
} else {
216+
break;
217+
}
218+
}
219+
}
220+
221+
while (j > 0) {
222+
array[--k] = buffer[--j];
223+
}
224+
}
225+
position += bucketSize;
226+
}
227+
position = 0;
228+
lastSize = bucketSize;
229+
bucketSize *= 2;
230+
}
231+
});
232+
233+
platform.registerChakraLibraryFunction("DefaultStringSortCompare", function(left, right) {
234+
// this version is used when the array was already strings
235+
// as the sort only ever checks for < 0 on the return value of compare functions
236+
// only have to handle this case
237+
if (left < right) {
238+
return -1;
239+
}
240+
return 0;
241+
});
242+
243+
platform.registerChakraLibraryFunction("DefaultSortCompare", function(left, right) {
244+
// as the sort only ever checks for < 0 on the return value of compare functions
245+
// only have to handle this case
246+
if (left.string < right.string) {
247+
return -1;
248+
}
249+
return 0;
250+
});
251+
252+
platform.registerChakraLibraryFunction("CreateCompareArray", function(array, length) {
253+
let useCompareArray = false;
254+
let i = 0;
255+
while (i < length) {
256+
if (typeof array[i++] !== "string") {
257+
useCompareArray = true;
258+
break;
259+
}
260+
}
261+
262+
if (useCompareArray === true) {
263+
const compArray = [];
264+
compArray.__proto__ = null;
265+
i = 0;
266+
let value;
267+
while (i < length) {
268+
value = array[i];
269+
compArray[i++] = {
270+
value : value,
271+
string : "" + value
272+
};
273+
}
274+
return compArray;
275+
}
276+
return array;
277+
});
278+
279+
platform.registerChakraLibraryFunction("FillArrayHoles", function(array, length, offset) {
280+
let i = offset, j = offset, holes = 0;
281+
let value;
282+
while (i < length) {
283+
value = array[i];
284+
if (value !== undefined) {
285+
array[j++] = value;
286+
} else if (!(i in array)) {
287+
++holes;
288+
}
289+
++i;
290+
}
291+
292+
const valuesLength = j;
293+
const hasLength = length - holes;
294+
while (j < hasLength) {
295+
array[j++] = undefined;
296+
}
297+
while (j < length) {
298+
delete array[j++];
299+
}
300+
return valuesLength;
301+
});
302+
303+
platform.registerFunction(platform.FunctionKind.Array_sort, function (compareFn) {
304+
//#sec-array.prototype.sort
305+
if (compareFn !== undefined && typeof compareFn !== "function") {
306+
__chakraLibrary.raiseFunctionArgument_NeedFunction("Array.prototype.sort");
307+
}
308+
309+
const {o, len} = __chakraLibrary.CheckArrayAndGetLen(this, "Array.prototype.sort");
310+
311+
if (len < 2) { // early return if length < 2
312+
return o;
313+
}
314+
315+
// check for if the array has any missing values
316+
// also pull in any values from the prototype
317+
let i = 0, length = len;
318+
while (i < len) {
319+
if (o[i] === undefined) {
320+
length = __chakraLibrary.FillArrayHoles(o, len, i);
321+
break;
322+
}
323+
o[i] = o[i++];
324+
}
325+
326+
let compArray = o;
327+
if (compareFn === undefined && length > 1) {
328+
compArray = __chakraLibrary.CreateCompareArray(o, length);
329+
if (compArray === o) {
330+
compareFn = __chakraLibrary.DefaultStringSortCompare;
331+
} else {
332+
compareFn = __chakraLibrary.DefaultSortCompare;
333+
}
334+
}
335+
336+
// for short arrays perform an insertion sort
337+
if (length < 2048) {
338+
let sortedCount = 1, lowerBound = 0, insertPoint = 0, upperBound = 0;
339+
while (sortedCount < length) {
340+
const item = compArray[sortedCount];
341+
upperBound = sortedCount;
342+
insertPoint = sortedCount - 1; // this lets us check for already ordered first
343+
lowerBound = 0;
344+
for (;;) {
345+
if (compareFn (item, compArray[insertPoint]) < 0) {
346+
upperBound = insertPoint;
347+
} else {
348+
lowerBound = insertPoint + 1;
349+
}
350+
if (lowerBound >= upperBound) {
351+
break;
352+
}
353+
insertPoint = lowerBound + (upperBound - lowerBound >> 1);
354+
}
355+
insertPoint = sortedCount;
356+
while (insertPoint > lowerBound) {
357+
compArray[insertPoint--] = compArray[insertPoint];
358+
}
359+
compArray[lowerBound] = item;
360+
++sortedCount;
361+
}
362+
} else {
363+
__chakraLibrary.MergeSort(compArray, length, compareFn);
364+
}
365+
366+
if (compArray !== o) {
367+
i = 0;
368+
while (i < length) {
369+
o[i] = compArray[i++].value;
370+
}
371+
}
372+
373+
return o;
374+
});
375+
177376
platform.registerFunction(platform.FunctionKind.Array_filter, function (callbackfn, thisArg = undefined) {
178377
// ECMAScript 2017 #sec-array.prototype.filter
179378

0 commit comments

Comments
 (0)