Skip to content

Use removeItem instead of copyListRemovingItem#10349

Merged
7 commits merged into
masterfrom
remove_item_from_list
Sep 1, 2016
Merged

Use removeItem instead of copyListRemovingItem#10349
7 commits merged into
masterfrom
remove_item_from_list

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Aug 15, 2016

If someone were accessing e.g. openFilesReferenced directly 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.

@ghost ghost force-pushed the remove_item_from_list branch from 95997f9 to 2522c9a Compare August 15, 2016 20:10
@ghost ghost assigned zhengbli Aug 15, 2016
@ghost ghost force-pushed the remove_item_from_list branch from 2522c9a to e13fe15 Compare August 15, 2016 20:12
@ghost ghost force-pushed the remove_item_from_list branch from e13fe15 to 5ad7729 Compare August 15, 2016 20:17
Comment thread src/compiler/core.ts
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)`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

measured how?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this collection add callbacks in the order they are triggered, the order should be preserved here.

Comment thread src/compiler/core.ts Outdated
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 {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistent naming make it orderedRemoveItemAt

@zhengbli
Copy link
Copy Markdown

zhengbli commented Sep 1, 2016

Other than the comments, LGTM 👍

@ghost ghost merged commit 2961d97 into master Sep 1, 2016
@ghost ghost deleted the remove_item_from_list branch September 1, 2016 19:50
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants