Skip to content

Add RwLock and concurrency enabled LinearMemory#136

Merged
wucke13 merged 4 commits intomainfrom
dev/add-rwspinlock
Mar 21, 2025
Merged

Add RwLock and concurrency enabled LinearMemory#136
wucke13 merged 4 commits intomainfrom
dev/add-rwspinlock

Conversation

@wucke13
Copy link
Copy Markdown
Collaborator

@wucke13 wucke13 commented Mar 10, 2025

Pull Request Overview

Adds:

  • RwSpinLock, a spinning, writer preferring read-write lock implementation
  • LinearMemory, a linear memory implementation that enable shared concurrent access

Testing Strategy

This pull request was tested by its including tests

TODO or Help

This pull request still needs a keen eye on the use of ordering for the atomic operations in the RwSpinLock implementation.

Formatting

  • Ran cargo fmt
  • Ran cargo check
  • Ran cargo build
  • Ran cargo doc
  • Ran nix fmt

Github Issue

This pull request continues the ideas laid out in #135

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 10, 2025

Codecov Report

Attention: Patch coverage is 94.11765% with 31 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/execution/linear_memory.rs 96.22% 8 Missing and 6 partials ⚠️
src/execution/interpreter_loop.rs 86.66% 0 Missing and 10 partials ⚠️
src/core/rw_spinlock.rs 91.17% 6 Missing ⚠️
src/execution/mod.rs 75.00% 0 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
src/core/little_endian.rs 100.00% <100.00%> (ø)
src/execution/store.rs 86.66% <100.00%> (-0.84%) ⬇️
src/lib.rs 27.17% <ø> (ø)
src/execution/mod.rs 93.48% <75.00%> (-0.06%) ⬇️
src/core/rw_spinlock.rs 91.17% <91.17%> (ø)
src/execution/interpreter_loop.rs 99.08% <86.66%> (+<0.01%) ⬆️
src/execution/linear_memory.rs 96.22% <96.22%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wucke13 wucke13 force-pushed the dev/add-rwspinlock branch from d8c6921 to 23116b1 Compare March 11, 2025 12:43
@eva-cosma
Copy link
Copy Markdown
Collaborator

The code looks good overall, but I would like a soundness check w/r/t to atomic ordering. I'm not super familiar with them, but I know of the importance of getting them right. Perhaps we can make this a topic for tomorrow's meeting

Also, for the spinlock, while I have no gripes of setting u32::MAX/2 as a soft-maximum and having u32::MAX as a "writer present flag", it isn't really that rust-y. A rogue +1 with the u32::MAX set can result in an underflow/panic. If we believe we are not going to touch this code in the near future, we can leave this as is, since it looks like a correct implementation. If not, having an inline function to turn the AtomicU32 (after get) into an enum variant that we then check, or the other way around (before set) can reduce the chance of a logic bug appearing.

@wucke13 wucke13 force-pushed the dev/add-rwspinlock branch 2 times, most recently from ad4dc3d to 1e6029f Compare March 14, 2025 11:07
@wucke13
Copy link
Copy Markdown
Collaborator Author

wucke13 commented Mar 17, 2025

Mara Bos describes a mechanism to favour writers without having wainting_writers atomic. Instead, readers always increment the lock by 2, and waiting writers by one. New reader that see an uneven state wait until the state becomes even again.

Further information: https://marabos.nl/atomics/building-locks.html#avoiding-writer-starvation

@wucke13 wucke13 force-pushed the dev/add-rwspinlock branch 6 times, most recently from beae551 to 5126052 Compare March 21, 2025 09:03
@wucke13
Copy link
Copy Markdown
Collaborator Author

wucke13 commented Mar 21, 2025

@george-cosma @cemonem I tried to apply your feedback, fix the bugs, improve the locking a bit further, now with ordering completely identical to the mara bos book. I also extended the documentation. From my point of view, this PR is good. Please have a look if you find any more gotchas 😄

@wucke13 wucke13 force-pushed the dev/add-rwspinlock branch from 5126052 to 640469b Compare March 21, 2025 14:19
wucke13 added 4 commits March 21, 2025 15:20
In preparation for support of the multiple linear memories proposal, we
will need locking. This commit introduces a first version of a naive
read-write lock implementation that solely relies on spinning, avoiding
any dependence on an operating system. The lock is tailored for the use
case in a linear memory which is shared between multiple, concurrently
running interpreter instances.

Signed-off-by: wucke13 <wucke13+github@gmail.com>
This trait provides methods to convert between values and their
canonical representation in little endian byte order. The trait is
implemented for all integer types and as well as for `f32` and `f64`.

Signed-off-by: wucke13 <wucke13+github@gmail.com>
This implementation of linear memory is capable of being shared between
multiple, concurrently running interpreter instances. All access to the
actual data goes through raw pointers, derived from `UnsafeCell`s. While
this allows race-conditions, it avoids the other kinds of UB that may
be caused by code that violates the guarantees attached to shared and
mutable references.

Signed-off-by: wucke13 <wucke13+github@gmail.com>
This commit integrates the new `LinearMemory` implementation into the
interpreter

Signed-off-by: wucke13 <wucke13+github@gmail.com>
@wucke13 wucke13 force-pushed the dev/add-rwspinlock branch from 640469b to 3212000 Compare March 21, 2025 14:20
@wucke13 wucke13 added this pull request to the merge queue Mar 21, 2025
Merged via the queue into main with commit c1098c3 Mar 21, 2025
11 checks passed
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