✅ Deploy Preview for salsa-rs canceled.

CodSpeed Performance Report

Merging #741 will improve performance by 7.17%

_{Comparing Veykril:veykril/push-ooqmumwlopur (9f47a9e) with master (9b62468)}

Summary

⚡ 2 improvements
✅ 11 untouched

Benchmarks breakdown

Does this fix an unsoundness because it otherwise seems to regress perf a little bit.

I believe (if I understand the code correctly) this fixes a theoretical unsoundness when a user leaks a tracked struct value across threads. In that scenario we might try to acquire the read lock and write lock at the same time which could cause aliasing (before we inevitably panic).

The perf regression likely comes from taking the read lock in tracked_field now. I believe we are required to take the lock there as otherwise the access could alias (given the above scenario).

I'll see if we can craft a test that actually hits that lock setup resulting in a panic (to check if miri might be able to diagnose the theoretical issue)

I think I'm running into the aliasing bug with Red Knot's WASM based playground.

We see instances where it panics with

access to field whilst the value is being initialized

I rebased your PR and I'm no longer seeing the same panic (or it at least makes it much less likely to reproduce 😆)

I forgot the specifics of what's left. Should I take this PR from you?

Just whatever is left regarding the review comments I've put up I think, though I am not sure that what you observe would be fixed by this PR (unless we are in fact running into UB!). Feel free to take it over, I'm busy this week with a rust-analyzer refactor

@ibraheemdev you're probably the better reviewer for this than I.

@Veykril it seems that this PR uses some unstable language features. We could upgrade the MSRV if that helps (I think we're now on 1.85)

That would, iirc raw borrows were stabilized in 1.81

Go ahead

Rebased and changed the revisions to be atomic. (things got simpler since last time I touched this PR)

I did not go with the UnsafeCell changes though, we can still do that later I guess.

I think this PR regressed memory usage for ty because, if I understand it correctly, we now store the memo_table pointer for each tracked struct. Is this necessary or could we retrieve the memo table on demand as we did before?

We stored it before too though

The Value and ValueWithMetadata struct split affects layouting though, so if you store a value with a big alignment I think this new setup will waste more space on unused padding. So might be good to check if we can undo that split again, which will make the code a bit more ugly though (or repr(packed) it?)