Skip to content

Comments

internal: Replace arc-swap with manual AtomicPtr#737

Merged
Veykril merged 3 commits intosalsa-rs:masterfrom
davidbarsky:davidbarsky/remove-arc-swap
Feb 26, 2025
Merged

internal: Replace arc-swap with manual AtomicPtr#737
Veykril merged 3 commits intosalsa-rs:masterfrom
davidbarsky:davidbarsky/remove-arc-swap

Conversation

@davidbarsky
Copy link
Contributor

This PR is a rebased version of #695; All credit for the changes goes to @ibraheemdev. As they put it:

It seems that we don't actually need arc-swap for the memo table, because our deleted_entries queue takes care of deferring reclamation until there are no live borrows of a given memo. arc-swap does a lot of extra bookkeeping; replacing it with a manual AtomicPtr object speeds up red-knot's incremental performance by about 15%. This should also make it easier to test with loom/shuttle.

You'll notice that none of the safety requirements of the MemoTable functions actually change with this PR, so I believe it is sound. The one catch is that we have to avoid returning ManuallyDrop<Box<T>> and instead work with NonNull until the allocation is actually freed and we can assert uniqueness, as Box is a noalias type (at least under the current rules).

I also played around with using boxcar::Vec instead of the current lock, but the lock is actually relatively uncontended and fine-grained and it was hard to balance cold and incremental performance with a lock-free implementation.

@netlify
Copy link

netlify bot commented Feb 26, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit da9a21c
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/67bf218dc666ab00083136fc

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 26, 2025

CodSpeed Performance Report

Merging #737 will improve performances by 11.02%

Comparing davidbarsky:davidbarsky/remove-arc-swap (da9a21c) with master (26aeeec)

Summary

⚡ 1 improvements
✅ 10 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
accumulator 4.2 ms 3.8 ms +11.02%

@Veykril Veykril enabled auto-merge February 26, 2025 14:21
@MichaReiser
Copy link
Contributor

Just FYI: You can push directly to PRs from contributors.

@Veykril Veykril added this pull request to the merge queue Feb 26, 2025
Merged via the queue into salsa-rs:master with commit 99be5d9 Feb 26, 2025
11 checks passed
@davidbarsky
Copy link
Contributor Author

Just FYI: You can push directly to PRs from contributors.

Ah, thanks! I always forget what the exact set of permissions are required for pushing to an existing PR.

@davidbarsky davidbarsky deleted the davidbarsky/remove-arc-swap branch February 26, 2025 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants