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

Replace orx-concurrent-vec with append-only-vec #555

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Aug 5, 2024

An alternative to #554

append-only-vec performs even better than boxcar and also helps to reduce the overall dependencies (and runs on 32bit!).

Copy link

netlify bot commented Aug 5, 2024

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 9fe3936
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/66b100dbdc0725000814776c

Copy link

codspeed-hq bot commented Aug 5, 2024

CodSpeed Performance Report

Merging #555 will improve performances by 12.16%

Comparing MichaReiser:append-only-vec (9fe3936) with master (d6df21f)

Summary

⚡ 1 improvements

Benchmarks breakdown

Benchmark master MichaReiser:append-only-vec Change
many_tracked_structs 147 µs 131.1 µs +12.16%

@MichaReiser MichaReiser marked this pull request as ready for review August 5, 2024 07:49
@nikomatsakis
Copy link
Member

Seems like a winner!

Cargo.toml Outdated Show resolved Hide resolved
@MichaReiser
Copy link
Contributor Author

Thanks for fixing the version. I'll merge this tomorrow unless you wanted me to address something else.

@nikomatsakis nikomatsakis enabled auto-merge August 5, 2024 16:39
@MichaReiser
Copy link
Contributor Author

Okay I can't merge this while changes are requested. @nikomatsakis would you mind having another look.

@nikomatsakis nikomatsakis added this pull request to the merge queue Aug 6, 2024
Merged via the queue into salsa-rs:master with commit b098f92 Aug 6, 2024
10 checks passed
@ibraheemdev
Copy link
Contributor

ibraheemdev commented Sep 23, 2024

FWIW append-only-vec isn't a great concurrent vector implementation as it relies heavily on spin locks. In particular, it holds a spin lock when allocating which is almost always a bad idea, but it also uses a spin lock to serialize insertion order. These not only hurt scalability but can also lead to unfortunate tail latency scenarios that are common with spin locks.

Of course, the upside is that array access is slightly cheaper (though looking closer this might not necessarily be the case because access always requires loading the contended length atomic) due to the extra ad-hoc synchronization, so if you expect writes to be relatively rare this may not be an issue.. but something to look out for.

@MichaReiser
Copy link
Contributor Author

Thanks @ibraheemdev for the in-depth explanation.

I do think the spin-lock is fine the way we use it (at least for ingredients) because all write paths go through a lock anyway.

@ibraheemdev
Copy link
Contributor

ibraheemdev commented Oct 3, 2024

Interesting, a single-writer concurrent vector could be implemented more efficiently in general.. but probably not worth it for Salsa.

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.

3 participants