From 91bab5588c60497e200fa89a846e136b695db510 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Z=C3=BCnd?= Date: Tue, 5 Jun 2018 14:57:27 +0200 Subject: [PATCH] [array] Use random middle element to determine pivot during sorting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL adds a "random state" to the Smi Root list and implements a basic Linear congruential pseudo random number generator in Torque. The RNG is used to determine the pivot element for sorting. This will prevent the worst cases for certain data layouts. Drive-by-fix: Make sorting of ranges and execution pauses for profviz deterministic by adding a secondary sorting criteria. Bug: v8:7382 Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng Change-Id: Ieb871e98e74bdb803f821b0cd35d2f67ee0f2868 Reviewed-on: https://chromium-review.googlesource.com/1082193 Reviewed-by: Hannes Payer Reviewed-by: Jakob Gruber Reviewed-by: Camillo Bruni Commit-Queue: Simon Zünd Cr-Commit-Position: refs/heads/master@{#53524} --- src/builtins/array.tq | 34 ++++++++++++++++++++++--- src/builtins/base.tq | 16 ++++++++++++ src/heap/heap.cc | 10 ++++++++ src/heap/heap.h | 3 ++- test/mjsunit/tools/profviz-test.default | 20 +++++++-------- test/test262/test262.status | 7 +++++ tools/profviz/composer.js | 13 +++++++--- 7 files changed, 85 insertions(+), 18 deletions(-) diff --git a/src/builtins/array.tq b/src/builtins/array.tq index 643ff79aa6..308fd4e2a7 100644 --- a/src/builtins/array.tq +++ b/src/builtins/array.tq @@ -637,16 +637,42 @@ module array { } } + // Returns a random positive Smi in the range of [0, range). + macro Rand(range: Smi): Smi { + assert(TaggedIsPositiveSmi(range)); + + let current_state: int32 = + LoadAndUntagToWord32Root(kArraySortRandomStateRootIndex); + let a: int32 = 1103515245; + let c: int32 = 12345; + let m: int32 = 0x3fffffff; // 2^30 bitmask. + + let new_state: int32 = ((current_state * a) + c) & m; + StoreRoot(kArraySortRandomStateRootIndex, convert(new_state)); + + let r: int32 = convert(range); + return convert(new_state % r); + } + macro CalculatePivot( context: Context, receiver: Object, elements: Object, initialReceiverMap: Object, initialReceiverLength: Number, from: Smi, to: Smi, userCmpFn: Object, sortCompare: CompareBuiltinFn): Object labels Bailout { - // TODO(szuend): Check if a more involved third_index calculation is - // worth it for very large arrays. - let third_index: Smi = from + ((to - from) >>> 1); + let random: Smi = Rand(to - from - 2); + assert(TaggedIsPositiveSmi(random)); - // Find a pivot as the median of first, last and middle element. + let third_index: Smi = from + 1 + random; + assert(third_index > from); + assert(third_index <= to - 1); + + // Find a pivot as the median of first, last and a random middle element. + // Always using the middle element as the third index causes the quicksort + // to degrade to O(n^2) for certain data configurations. + // The previous solution was to sample larger arrays and use the median + // element of the sorted sample. This causes more overhead than just + // choosing a random middle element, which also mitigates the worst cases + // in all relevant benchmarks. let v0: Object = Load(context, elements, from) otherwise Bailout; let v1: Object = Load(context, elements, to - 1) otherwise Bailout; let v2: Object = Load(context, elements, third_index) otherwise Bailout; diff --git a/src/builtins/base.tq b/src/builtins/base.tq index d8f6942a56..3f9e621416 100644 --- a/src/builtins/base.tq +++ b/src/builtins/base.tq @@ -53,6 +53,7 @@ type LanguageMode generates 'TNode' constexpr 'LanguageMode'; type ExtractFixedArrayFlags generates 'TNode' constexpr 'ExtractFixedArrayFlags'; type ParameterMode generates 'TNode' constexpr 'ParameterMode'; +type RootListIndex generates 'TNode' constexpr 'Heap::RootListIndex'; type MessageTemplate constexpr 'MessageTemplate'; type HasPropertyLookupMode constexpr 'HasPropertyLookupMode'; @@ -131,6 +132,9 @@ const kSloppy: constexpr LanguageMode = 'LanguageMode::kSloppy'; const SMI_PARAMETERS: constexpr ParameterMode = 'SMI_PARAMETERS'; const INTPTR_PARAMETERS: constexpr ParameterMode = 'INTPTR_PARAMETERS'; +const kArraySortRandomStateRootIndex: constexpr RootListIndex = + 'Heap::kArraySortRandomStateRootIndex'; + extern macro Print(Object); extern macro DebugBreak(); extern macro ToInteger_Inline(Context, Object): Number; @@ -156,6 +160,10 @@ extern runtime CreateDataProperty(Context, Object, Object, Object); extern runtime SetProperty(Context, Object, Object, Object, LanguageMode); extern runtime DeleteProperty(Context, Object, Object, LanguageMode); +extern macro LoadRoot(constexpr RootListIndex): Object; +extern macro StoreRoot(constexpr RootListIndex, Object): Object; +extern macro LoadAndUntagToWord32Root(constexpr RootListIndex): int32; + extern runtime StringEqual(Context, String, String): Oddball; extern builtin StringLessThan(Context, String, String): Boolean; @@ -208,12 +216,18 @@ extern operator '!=' macro WordNotEqual(Object, Object): bool; extern operator '+' macro SmiAdd(Smi, Smi): Smi; extern operator '-' macro SmiSub(Smi, Smi): Smi; +extern operator '&' macro SmiAnd(Smi, Smi): Smi; extern operator '>>>' macro SmiShr(Smi, constexpr int31): Smi; extern operator '+' macro IntPtrAdd(intptr, intptr): intptr; extern operator '-' macro IntPtrSub(intptr, intptr): intptr; extern operator '>>>' macro WordShr(intptr, intptr): intptr; +extern operator '+' macro Int32Add(int32, int32): int32; +extern operator '*' macro Int32Mul(int32, int32): int32; +extern operator '%' macro Int32Mod(int32, int32): int32; +extern operator '&' macro Word32And(int32, int32): int32; + extern operator '+' macro NumberAdd(Number, Number): Number; extern operator '-' macro NumberSub(Number, Number): Number; extern operator 'min' macro NumberMin(Number, Number): Number; @@ -235,6 +249,7 @@ extern operator extern operator 'is' macro TaggedIsSmi(Object): bool; extern operator 'isnt' macro TaggedIsNotSmi(Object): bool; +extern macro TaggedIsPositiveSmi(Object): bool; extern operator 'cast<>' macro TaggedToJSDataView(Object): JSDataView labels CastError; @@ -277,6 +292,7 @@ extern operator 'convert<>' macro TruncateWordToWord32(intptr): int32; extern operator 'convert<>' macro SmiTag(intptr): Smi; extern operator 'convert<>' macro SmiFromInt32(int32): Smi; extern operator 'convert<>' macro SmiUntag(Smi): intptr; +extern operator 'convert<>' macro SmiToInt32(Smi): int32; extern operator 'convert<>' macro LoadHeapNumberValue(HeapNumber): float64; extern operator 'convert<>' macro ChangeNumberToFloat64(Number): float64; diff --git a/src/heap/heap.cc b/src/heap/heap.cc index 5e3821cb46..d272b1283b 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -4709,6 +4709,16 @@ bool Heap::SetUp() { DCHECK_EQ(Smi::kZero, hash_seed()); if (FLAG_randomize_hashes) InitializeHashSeed(); + // Set up the random state that is used for random number generation in + // Torque. + int array_sort_random_seed = FLAG_random_seed; + if (array_sort_random_seed == 0) { + array_sort_random_seed = isolate()->random_number_generator()->NextInt(); + } + // Cut the seed to valid Smi range. + array_sort_random_seed &= ((static_cast(1) << kSmiShiftSize) - 1); + set_array_sort_random_state(Smi::FromInt(array_sort_random_seed)); + for (int i = 0; i < static_cast(v8::Isolate::kUseCounterFeatureCount); i++) { deferred_counters_[i] = 0; diff --git a/src/heap/heap.h b/src/heap/heap.h index b3cb4ec84b..84128f8aa8 100644 --- a/src/heap/heap.h +++ b/src/heap/heap.h @@ -300,7 +300,8 @@ using v8::MemoryPressureLevel; ConstructStubCreateDeoptPCOffset) \ V(Smi, construct_stub_invoke_deopt_pc_offset, \ ConstructStubInvokeDeoptPCOffset) \ - V(Smi, interpreter_entry_return_pc_offset, InterpreterEntryReturnPCOffset) + V(Smi, interpreter_entry_return_pc_offset, InterpreterEntryReturnPCOffset) \ + V(Smi, array_sort_random_state, ArraySortRandomState) #define ROOT_LIST(V) \ STRONG_ROOT_LIST(V) \ diff --git a/test/mjsunit/tools/profviz-test.default b/test/mjsunit/tools/profviz-test.default index 040afb4217..0c33e11abd 100644 --- a/test/mjsunit/tools/profviz-test.default +++ b/test/mjsunit/tools/profviz-test.default @@ -1435,8 +1435,8 @@ "46.74 0.08499999999999375", "57.64849999999997 0.08249999999999602", "58.316999999999965 0.08099999999999596", - "23.506 0.08050000000000068", "46.37200000000001 0.08050000000000068", + "23.506 0.08050000000000068", "42.70600000000002 0.07900000000000063", "129.4124999999999 0.07800000000000296", "20.5975 0.07750000000000057", @@ -1450,16 +1450,16 @@ "44.917000000000016 0.07200000000000273", "25.591500000000003 0.07199999999999918", "50.62049999999999 0.07150000000000034", - "46.621 0.07099999999999795", "88.82299999999992 0.07099999999999795", + "46.621 0.07099999999999795", "78.23049999999994 0.0660000000000025", "46.060500000000005 0.0659999999999954", "50.43099999999999 0.06400000000000006", "129.48849999999987 0.06349999999997635", "45.55900000000001 0.06150000000000233", "19.152 0.06050000000000111", - "50.20799999999999 0.060499999999997556", "57.33299999999997 0.060499999999997556", + "50.20799999999999 0.060499999999997556", "68.76649999999995 0.06049999999999045", "23.5775 0.059499999999999886", "47.135000000000005 0.05850000000000222", @@ -1468,8 +1468,8 @@ "21.2695 0.057500000000000995", "50.14149999999999 0.05749999999999744", "91.96649999999993 0.056500000000013983", - "57.934999999999974 0.05649999999999977", "83.63999999999993 0.05649999999999977", + "57.934999999999974 0.05649999999999977", "132.92249999999987 0.05649999999997135", "67.59199999999996 0.056000000000011596", "99.92199999999991 0.055499999999995", @@ -1490,9 +1490,9 @@ "75.46799999999995 0.04950000000000898", "45.76150000000001 0.049500000000001876", "94.6054999999999 0.04949999999998056", - "45.97850000000001 0.048500000000004206", - "115.4124999999999 0.048500000000004206", "118.19199999999991 0.048500000000004206", + "115.4124999999999 0.048500000000004206", + "45.97850000000001 0.048500000000004206", "49.780499999999996 0.0484999999999971", "42.795000000000016 0.04800000000000182", "126.59899999999989 0.04749999999999943", @@ -1512,23 +1512,23 @@ "52.15399999999998 0.0379999999999967", "42.88200000000001 0.0379999999999967", "24.430500000000002 0.03750000000000142", - "23.907 0.036499999999996646", "60.08349999999996 0.036499999999996646", + "23.907 0.036499999999996646", "50.32899999999999 0.036000000000001364", "42.31450000000002 0.034000000000006025", "45.02900000000001 0.032999999999994145", "23.189 0.031500000000001194", "21.49 0.03049999999999997", - "42.83300000000001 0.030000000000001137", "58.12149999999997 0.030000000000001137", "45.41750000000002 0.030000000000001137", + "42.83300000000001 0.030000000000001137", "140.89599999999987 0.028999999999996362", "2.4490000000000003 0.028500000000000636", "52.31499999999998 0.027999999999998693", "45.17200000000002 0.027999999999998693", - "43.632500000000014 0.027000000000001023", - "49.8685 0.027000000000001023", "51.30249999999999 0.027000000000001023", + "49.8685 0.027000000000001023", + "43.632500000000014 0.027000000000001023", "21.175 0.026499999999998636", "44.82200000000002 0.026000000000003354", "22.528 0.02599999999999625", diff --git a/test/test262/test262.status b/test/test262/test262.status index bcabe0b4e5..b1aa745ccb 100644 --- a/test/test262/test262.status +++ b/test/test262/test262.status @@ -454,6 +454,13 @@ 'built-ins/TypedArrays/ctors/buffer-arg/buffer-arg-bufferbyteoffset-throws-from-modulo-element-size': [FAIL], 'built-ins/TypedArrays/ctors/buffer-arg/buffer-arg-byteoffset-throws-from-modulo-element-size': [FAIL], + # Tests assume that the sort order of "same elements" (comparator returns 0) + # is deterministic. + # https://bugs.chromium.org/p/v8/issues/detail?id=7808 + 'intl402/String/prototype/localeCompare/returns-same-results-as-Collator': [SKIP], + 'intl402/Collator/prototype/compare/bound-to-collator-instance': [SKIP], + 'intl402/Collator/ignore-invalid-unicode-ext-values': [SKIP], + ######################## NEEDS INVESTIGATION ########################### # These test failures are specific to the intl402 suite and need investigation diff --git a/tools/profviz/composer.js b/tools/profviz/composer.js index ce625addca..3e0b33f343 100644 --- a/tools/profviz/composer.js +++ b/tools/profviz/composer.js @@ -176,7 +176,9 @@ function PlotScriptComposer(kResX, kResY, error_output) { } function MergeRanges(ranges) { - ranges.sort(function(a, b) { return a.start - b.start; }); + ranges.sort(function(a, b) { + return (a.start == b.start) ? a.end - b.end : a.start - b.start; + }); var result = []; var j = 0; for (var i = 0; i < ranges.length; i = j) { @@ -516,8 +518,13 @@ function PlotScriptComposer(kResX, kResY, error_output) { // Label the longest pauses. execution_pauses = RestrictRangesTo(execution_pauses, range_start, range_end); - execution_pauses.sort( - function(a, b) { return b.duration() - a.duration(); }); + execution_pauses.sort(function(a, b) { + if (a.duration() == b.duration() && b.end == a.end) + return b.start - a.start; + + return (a.duration() == b.duration()) + ? b.end - a.end : b.duration() - a.duration(); + }); var max_pause_time = execution_pauses.length > 0 ? execution_pauses[0].duration() : 0;