Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: use map for sting-based stores #383

Merged
merged 1 commit into from
Feb 18, 2025
Merged

Conversation

gurgunday
Copy link
Contributor

@gurgunday gurgunday commented Dec 5, 2024

When an object is used as a map and has string-based keys, Map's performance will be superior

For this repo, not sure how to test it but here's some related discussion and benchmarks in kibertoad/toad-cache#40

And a V8 engineer explaining why: https://stackoverflow.com/questions/62350146/why-map-manipulation-is-much-slower-than-object-in-javascript-v8-for-integ/62351925#62351925

@mcollina
Copy link
Collaborator

mcollina commented Dec 6, 2024

Did you run the benchmarks? Where there any improvements?

@gurgunday
Copy link
Contributor Author

I will post a benchmark if I can

@gurgunday
Copy link
Contributor Author

Yeah no significant improvement here

@gurgunday gurgunday closed this Dec 13, 2024
@gurgunday
Copy link
Contributor Author

I don't believe this didn't improve anything, I'll take another look 🤣

@gurgunday gurgunday reopened this Feb 17, 2025
@gurgunday
Copy link
Contributor Author

gurgunday commented Feb 17, 2025

@mcollina so I ran some tests again

For keys that aren't integers, Map is still consistently faster, it also doesn't force V8 to reconsider the hidden class a bunch of times

We had the same results in toad-cache, so it's perfectly safe to land

We don't have a benchmark that tests these because they are only used in Constrainer, but here is a generic benchmark:

Map time for 1000 iterations: 0.90 ms
Object time for 1000 iterations: 1.60 ms
function bench(iterations) {
    const map = new Map();
    const obj = {};

    for (let i = 1; i <= 10; i++) {
        map.set(`${i}.x.x`, `host${i}`);
        obj[`${i}.x.x`] = `host${i}`;
    }

    function benchmarkMap() {
        let sum = 0;
        for (let i = 0; i < iterations; i++) {
            for (let j = 1; j <= 10; j++) {
                sum += map.get(`${j}.x.x`).length;
            }
        }
        return sum;
    }

    function benchmarkObject() {
        let sum = 0;
        for (let i = 0; i < iterations; i++) {
            for (let j = 1; j <= 10; j++) {
                sum += obj[`${j}.x.x`].length;
            }
        }
        return sum;
    }

    const startMap = performance.now();
    benchmarkMap();
    const endMap = performance.now();

    const startObject = performance.now();
    benchmarkObject();
    const endObject = performance.now();

    console.log(`Map time for ${iterations} iterations:`, (endMap - startMap).toFixed(2), "ms");
    console.log(`Object time for ${iterations} iterations:`, (endObject - startObject).toFixed(2), "ms");
}

bench(1000);

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 0c3b431 into delvedor:main Feb 18, 2025
7 checks passed
@gurgunday gurgunday deleted the map branch February 18, 2025 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants