feat: upgrade remove duplicates performance

[xixileng]

No description provided.

[yar2001]

Thanks! This version improves the performance greatly when there are a lot of elements concentrated in one grid. The original use of filter and indexOf was not performance-friendly enough.

Thank you for your library, @timohausmann. Can you review this PR?

[timohausmann]

Thanks for the PR and reminder. Can you provide an example where this improves performance? In the many to many example I get ~10 fps less with 6400 objects.

My first thought on refactoring duplicate removal would be a Set like return [...new Set(returnObjects)]; but surprisingly this is slower than the filter, too.

The check if(this.level === 0) without the WeakMap nearly doubles performance though, lol. I think it makes sense to only do this once and not on every node, so I can see merging that.

[xixileng]

Yes, if(this.level === 0) is important, and using a WeakMap can reduce the complexity of removing duplicates from O(n^2) to O(n). The reason you get fewer FPS is due to the draw() function(because retrieve is more fast LoL). We should only compare the performance of the retrieve function.

here is an example:

// simple script.js

// Log cost time
console.time("retrieve");
const candidates = tree.retrieve(myCursor);
console.timeEnd("retrieve");

// Add many objects
document.getElementById("btnAdd10").addEventListener("click", function () {
  for (var i = 0; i < 1000000; i++) {
    handleAdd();
  }
});

// Here, disable render to avoid side effects.
function draw() {
  return;
}

Then, when comparing the two retrieve functions, I believe you will be able to see the difference in the console.

[xixileng]

I have optimized the deduplication logic again. The current logic is clearer, and the test speed has slightly improved compared to the previous version I submitted (the complexity remains the same, perhaps because some data access has been reduced).

[timohausmann]

Okay right, we should measure retrieve function only. Here is what I got:

Filter:  ran 100 times, average: 30.092999999970196 ms
WeakMap: ran 100 times, average: 0.8009999999403954 ms
Set:     ran 100 times, average: 0.7029999999701977 ms

That's a great improvement!

Can your WeakMap beat Set though? It's basically a one-liner:

if(this.level === 0) {
    return [... new Set(returnObjects)];
}

Performance script:

const objectAmount = 1000000;
const times = 100;
let all = 0;

for(var i=0;i<objectAmount;i++) { 
    handleAdd();
};

for(let i=0; i<times; i++) {
    // console.time("retrieve");
    const start = window.performance.now();
    tree.retrieve(myCursor);
    // console.timeEnd("retrieve");
    const end = window.performance.now();
    const time = end - start;
    // console.log('retrieve', time);
    all += time;
}

console.log('ran', times, 'times, average:', (all / times), 'ms')

[xixileng]

Okay, I'm using Array.from(new Set(returnObjects)) because [...new Set(returnObjects)] generates a warning: Type 'Set<ObjectsType>' is not an array type or a string type. Use compiler option '--downlevelIteration' to allow iterating of iterators. However, I don't want to change the tsconfig, so Array.from is a good solution.

[timohausmann]

Good stuff, thanks.
I merged it and published a new version to npm: https://www.npmjs.com/package/@timohausmann/quadtree-ts