Use removeItem instead of copyListRemovingItem#10349
Conversation
95997f9 to
2522c9a
Compare
2522c9a to
e13fe15
Compare
e13fe15 to
5ad7729
Compare
| copiedList.push(e); | ||
| /** Remove an item from an array, moving everything to its right one space left. */ | ||
| export function removeItemAt<T>(array: T[], index: number): void { | ||
| // This seems to be faster than either `array.splice(i, 1)` or `array.copyWithin(i, i+ 1)`. |
There was a problem hiding this comment.
const assert = require("assert");
const {Suite} = require("benchmark");
const pad = require("pad");
// Implementations
function loop(array, index) {
for (let i = index; i < array.length - 1; i++) {
array[i] = array[i + 1];
}
array.pop();
}
function splice(array, index) {
array.splice(index, 1);
}
function memcpy(array, index) {
array.copyWithin(index, index + 1);
array.pop();
}
const fns = [loop, splice, memcpy];
// Correctness check
function randint(min, max) {
return min + Math.floor(Math.random() * (max - min));
}
for (const f of fns) {
const arr = sampleArray(100);
for (let len = arr.length; len > 0; len--) {
const i = randint(0, len);
const em = arr[i];
assert(arr.indexOf(em) !== -1);
f(arr, i);
assert(arr.indexOf(em) === -1);
assert.equal(arr.length, len - 1);
}
}
// Test helpers
function sampleArray(N) {
const arr = new Array(N);
for (let i = 0; i < N; i++) {
arr[i] = i;
}
return arr;
}
function testFront(n, remove) {
const arr = sampleArray(n);
for (let len = arr.length; len > 0; len--) {
remove(arr, 0);
assert(arr.length === len - 1);
}
}
function testRear(n, remove) {
const arr = sampleArray(n);
for (let len = arr.length; len > 0; len--) {
remove(arr, len - 1);
assert(arr.length === len - 1);
}
}
// Perf test
function test(makeSuite) {
const suite = new Suite();
makeSuite(suite);
suite.on("complete", function() {
this.forEach(({name, stats}) => {
console.log(`${name}: ${stats.mean * 1000}ms`);
});
});
suite.on("error", error => {
throw error.target.error;
});
suite.run();
}
test(suite => {
for (const test of [testFront, testRear]) {
for (const n of [10, 100, 1000]) {
for (const f of fns) {
suite.add(`${pad(9, test.name)} ${pad(4, n)} ${pad(6, f.name)}`, () => test(n, f));
}
}
}
});
Results:
testFront 10 loop: 0.00028075305868859946ms
testFront 10 splice: 0.0024167716220477586ms
testFront 10 memcpy: 0.003104232232456715ms
testFront 100 loop: 0.014285475240188552ms
testFront 100 splice: 0.023001260699088717ms
testFront 100 memcpy: 0.28665747186761226ms
testFront 1000 loop: 1.1882795027214252ms
testFront 1000 splice: 0.23387040256038646ms
testFront 1000 memcpy: 26.856770945312498ms
testRear 10 loop: 0.0001227723598808805ms
testRear 10 splice: 0.002122261221082299ms
testRear 10 memcpy: 0.0004173716600574439ms
testRear 100 loop: 0.001598535853706533ms
testRear 100 splice: 0.021219825430875625ms
testRear 100 memcpy: 0.0037203930576036157ms
testRear 1000 loop: 0.015465716346486885ms
testRear 1000 splice: 0.21165966573636746ms
testRear 1000 memcpy: 0.03714556813039422ms
loop is best for all but testFront 1000, which is a pretty unlikely scenario.
There was a problem hiding this comment.
For really long comments, there's https://gist.github.com ;)
| readonly clearTimeout = (timeoutId: any): void => { | ||
| if (typeof timeoutId === "number") { | ||
| this.callbackQueue.splice(timeoutId, 1); | ||
| unorderedRemoveItemAt(this.callbackQueue, timeoutId); |
There was a problem hiding this comment.
This is called a "queue" but we never shift off the front; we just run all the callbacks with for-of. Still, it's possible that callers of setTimeout expect their callbacks to run in the order they were added. Should we change this to removeItemAtPreservingOrder?
There was a problem hiding this comment.
Yes, I think this collection add callbacks in the order they are triggered, the order should be preserved here.
| if (e !== item) { | ||
| copiedList.push(e); | ||
| /** Remove an item from an array, moving everything to its right one space left. */ | ||
| export function removeItemAtPreservingOrder<T>(array: T[], index: number): void { |
There was a problem hiding this comment.
for consistent naming make it orderedRemoveItemAt
|
Other than the comments, LGTM 👍 |
If someone were accessing e.g.
openFilesReferenceddirectly and expecting it to never change this would break their code. We might want to make these private or document that they will mutably update.