Skip to content
forked from v8/v8

Commit

Permalink
[set-methods] Fix symmetricDifference fast path
Browse files Browse the repository at this point in the history
This CL fixes two bugs.

One, propagate newly allocated OrderedHashSets to the next iteration of
the loop in the fast path. Set.p.symmetricDifference can add as well as
remove elements from the result OrderedHashSet.  Adding elements can
result in a newly allocated OrderedHashSet.

Two, start counting the number of elements at the size of the receiver
set instead of 0. The initial result is a copy of the receiver set.

Bug: v8:13556
Change-Id: Ieac44eae38ed0592fe1e486ce1fc0824699f36ea
Fixed: chromium:1470448
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4754678
Reviewed-by: Rezvan Mahdavi Hezaveh <rezvan@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#89415}
  • Loading branch information
syg authored and V8 LUCI CQ committed Aug 7, 2023
1 parent 8f1f31c commit aeb4552
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 23 deletions.
61 changes: 38 additions & 23 deletions src/builtins/set-symmetric-difference.tq
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,14 @@ transitioning javascript builtin SetPrototypeSymmetricDifference(
GetKeysIterator(otherRec.object, UnsafeCast<Callable>(otherRec.keys));

// 5. Let resultSetData be a copy of O.[[SetData]].
let resultSetData = Cast<OrderedHashSet>(CloneFixedArray(
const resultSetData = Cast<OrderedHashSet>(CloneFixedArray(
table, ExtractFixedArrayFlag::kFixedArrays)) otherwise unreachable;
let resultAndNumberOfElements = OrderedHashSetAndNumberOfElements{
setData: resultSetData,
numberOfElements: UnsafeCast<Smi>(
resultSetData.objects[kOrderedHashSetNumberOfElementsIndex])
};

let numberOfElements: Smi = 0;
try {
typeswitch (other) {
case (otherSet: JSSetWithNoCustomIteration): {
Expand All @@ -44,8 +48,8 @@ transitioning javascript builtin SetPrototypeSymmetricDifference(
while (true) {
const nextValue = otherIterator.Next() otherwise Done;

numberOfElements = FastSymmetricDifference(
nextValue, table, resultSetData, numberOfElements, methodName);
resultAndNumberOfElements = FastSymmetricDifference(
nextValue, table, resultAndNumberOfElements, methodName);
}
}
case (otherMap: JSMapWithNoCustomIteration): {
Expand All @@ -60,9 +64,8 @@ transitioning javascript builtin SetPrototypeSymmetricDifference(
while (true) {
const nextValue = otherIterator.Next() otherwise Done;

numberOfElements = FastSymmetricDifference(
nextValue.key, table, resultSetData, numberOfElements,
methodName);
resultAndNumberOfElements = FastSymmetricDifference(
nextValue.key, table, resultAndNumberOfElements, methodName);
}
}
case (JSAny): {
Expand All @@ -87,46 +90,55 @@ transitioning javascript builtin SetPrototypeSymmetricDifference(
nextValue = collections::NormalizeNumberKey(nextValue);

// iii. Let inResult be SetDataHas(resultSetData, nextValue).
const inResult = TableHasKey(resultSetData, nextValue);
const inResult =
TableHasKey(resultAndNumberOfElements.setData, nextValue);

// iv. If SetDataHas(O.[[SetData]], nextValue) is true, then
if (TableHasKey(table, nextValue)) {
// 1. If inResult is true, remove nextValue from resultSetData.
if (inResult) {
numberOfElements = DeleteFromSetTable(resultSetData, nextValue)
resultAndNumberOfElements.numberOfElements =
DeleteFromSetTable(resultAndNumberOfElements.setData, nextValue)
otherwise unreachable;
}
} else {
// v. Else,
// 1. If inResult is false, append nextValue to resultSetData.
if (!inResult) {
resultSetData = AddToSetTable(resultSetData, nextValue, methodName);
numberOfElements++;
resultAndNumberOfElements.setData = AddToSetTable(
resultAndNumberOfElements.setData, nextValue, methodName);
resultAndNumberOfElements.numberOfElements++;
}
}
}
} label Done {
resultSetData =
ShrinkOrderedHashSetIfNeeded(numberOfElements, resultSetData);
const shrunk = ShrinkOrderedHashSetIfNeeded(
resultAndNumberOfElements.numberOfElements,
resultAndNumberOfElements.setData);
return new JSSet{
map: *NativeContextSlot(ContextSlot::JS_SET_MAP_INDEX),
properties_or_hash: kEmptyFixedArray,
elements: kEmptyFixedArray,
table: resultSetData
table: shrunk
};
}
unreachable;
}

// This macro gets the nextValue in other table and normalize it. If the
// nextValue exists in the reciever table, it will be removed. Otherwise
// nextValue exists in the receiver table, it will be removed. Otherwise
// it will be added to the resultSetData.
struct OrderedHashSetAndNumberOfElements {
setData: OrderedHashSet;
numberOfElements: Smi;
}
macro FastSymmetricDifference(implicit context: Context)(
nextValue: JSAny, table: OrderedHashSet, resultSetData: OrderedHashSet,
numberOfElements: Smi, methodName: constexpr string): Smi {
nextValue: JSAny, table: OrderedHashSet,
resultSetDataAndNumberOfElements: OrderedHashSetAndNumberOfElements,
methodName: constexpr string): OrderedHashSetAndNumberOfElements {
let key = nextValue;
let result = resultSetData;
let numberOfElementsModifiable = numberOfElements;
let resultSetData = resultSetDataAndNumberOfElements.setData;
let numberOfElements = resultSetDataAndNumberOfElements.numberOfElements;

// ii. If nextValue is -0𝔽, set nextValue to +0𝔽.
key = collections::NormalizeNumberKey(key);
Expand All @@ -138,14 +150,17 @@ macro FastSymmetricDifference(implicit context: Context)(
dcheck(inResult == TableHasKey(table, key));
// 1. If inResult is true, remove nextValue from resultSetData.
if (inResult) {
numberOfElementsModifiable = DeleteFromSetTable(resultSetData, key)
numberOfElements = DeleteFromSetTable(resultSetData, key)
otherwise unreachable;
} else {
// v. Else,
// 1. If inResult is false, append nextValue to resultSetData.
result = AddToSetTable(resultSetData, key, methodName);
numberOfElementsModifiable++;
resultSetData = AddToSetTable(resultSetData, key, methodName);
numberOfElements++;
}
return numberOfElementsModifiable;
return OrderedHashSetAndNumberOfElements{
setData: resultSetData,
numberOfElements: numberOfElements
};
}
}
16 changes: 16 additions & 0 deletions test/mjsunit/harmony/set-symmetric-difference.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,3 +200,19 @@

assertEquals(resultArray, symmetricDifferenceArray);
})();

(function TestSymmetricDifferenceGrowFastPath() {
const s1 = new Set([1, 2, 3]).symmetricDifference(new Set([4, 5, 6]));
assertEquals([1, 2, 3, 4, 5, 6], Array.from(s1));

const s2 = new Set([1, 2, 3]).symmetricDifference(new Set([3, 4, 5, 6]));
assertEquals([1, 2, 4, 5, 6], Array.from(s2));

const s3 =
new Set([1, 2, 3]).symmetricDifference(new Map([[4, 4], [5, 5], [6, 6]]));
assertEquals([1, 2, 3, 4, 5, 6], Array.from(s3));

const s4 = new Set([1, 2, 3]).symmetricDifference(
new Map([[3, 3], [4, 4], [5, 5], [6, 6]]));
assertEquals([1, 2, 4, 5, 6], Array.from(s4));
})();

0 comments on commit aeb4552

Please sign in to comment.