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

Add optional support for write locks #16

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

austinhartzheim
Copy link
Collaborator

@austinhartzheim austinhartzheim commented Sep 12, 2024

Write locks prevent accidental misuse of Synchronizers by establishing an advisory lock on the state file using the flock system call. Correctly configured writers will check this lock to prevent duplicate writers.

Write locking functionality is enabled by choosing a WriteLockStrategy and configuring the WL generic parameter on Synchronizer.

Design

This code is designed to be zero-cost when write locking is disabled. By using a generic parameter, the compiler optimizes away the LockDisabled implementation during monomorphization.

Currently, we only support LockDisabled and SingleWriter strategies, where SingleWriter makes a non-blocking call to flock to establish a lock. Future work might add support for multiple writers (i.e., where the lock is released after every write so other threads/processes can write), in addition to blocking/non-blocking variations of single- and multiple-writer modes.

Internally, the state method has been divided into state_read and state_write to avoid an additional branch condition for reads. This design allows read throughout to remain unchanged.

Benchmarks

synchronizer/write      time:   [427.18 ns 428.05 ns 428.98 ns]
                        thrpt:  [2.3311 Melem/s 2.3362 Melem/s 2.3410 Melem/s]
                 change:
                        time:   [-1.8225% -1.2920% -0.9011%] (p = 0.00 < 0.05)
                        thrpt:  [+0.9093% +1.3089% +1.8563%]
                        Change within noise threshold.
synchronizer/write_raw  time:   [281.40 ns 282.29 ns 283.15 ns]
                        thrpt:  [3.5317 Melem/s 3.5424 Melem/s 3.5536 Melem/s]
                 change:
                        time:   [-2.1847% -1.8092% -1.4802%] (p = 0.00 < 0.05)
                        thrpt:  [+1.5025% +1.8425% +2.2335%]
                        Performance has improved.
synchronizer/read/check_bytes_true
                        time:   [47.523 ns 47.634 ns 47.740 ns]
                        thrpt:  [20.947 Melem/s 20.993 Melem/s 21.043 Melem/s]
                 change:
                        time:   [-2.5116% -2.0644% -1.6443%] (p = 0.00 < 0.05)
                        thrpt:  [+1.6718% +2.1079% +2.5763%]
                        Performance has improved.
synchronizer/read/check_bytes_false
                        time:   [21.208 ns 21.256 ns 21.303 ns]
                        thrpt:  [46.942 Melem/s 47.046 Melem/s 47.151 Melem/s]
                 change:
                        time:   [-6.3104% -6.0734% -5.8230%] (p = 0.00 < 0.05)
                        thrpt:  [+6.1831% +6.4661% +6.7354%]
                        Performance has improved.

synchronizer_locked_read/disabled
                        time:   [21.457 ns 21.493 ns 21.525 ns]
                        thrpt:  [46.458 Melem/s 46.527 Melem/s 46.604 Melem/s]
synchronizer_locked_read/single_writer
                        time:   [21.172 ns 21.212 ns 21.253 ns]
                        thrpt:  [47.051 Melem/s 47.143 Melem/s 47.231 Melem/s]

synchronizer_locked_write/disabled
                        time:   [419.62 ns 420.54 ns 421.46 ns]
                        thrpt:  [2.3727 Melem/s 2.3779 Melem/s 2.3831 Melem/s]
synchronizer_locked_write/single_writer
                        time:   [450.42 ns 451.28 ns 452.07 ns]
                        thrpt:  [2.2121 Melem/s 2.2159 Melem/s 2.2201 Melem/s]

@austinhartzheim
Copy link
Collaborator Author

austinhartzheim commented Sep 12, 2024

Also, with this change, I took the liberty of updating dependencies, mostly to get a patch for ahash which was unable to build on nightly compiler versions. But also, this will pick up some patches for rkyv.

(We should evaluate whether it makes sense to continue shipping a Cargo.lock file for this library.)

pub fn new(path_prefix: &'a OsStr) -> Self {
Self {
path_prefix,
write_lock_mode: WriteLockMode::Disabled,
Copy link

Choose a reason for hiding this comment

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

Thinking out loud - what if default write mode was SingleWriter - (aka why would a caller build a Synchronizer if the default initialization results in a no-op)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you clarify what you mean? Are you suggesting that WriteLockMode::SingleWriter should be the default at all times (i.e., that we forego the write-lock feature flag)? Or that it should become the default automatically when the write-lock feature is enabled?

There are a few reasons this functionality is not the default and is gated behind a feature flag:

  1. Performance. We want to maintain zero-cost abstractions, or in Stroustrup's words "What you don't use, you don't pay for." @bocharov optimized writer performance in questions regarding low req/s write and how to sync between them as ipc use #11; by placing this code behind a feature flag we avoid performance regression for applications that do not require write locking.
  2. Compatibility. flock is only available on Unix systems, so the libc dependency and all calls to it must be gated behind a feature flag to retain the Windows support added in Windows support #3. (In theory, we may be able to add some Windows-compatible locking mechanism in the future; but I'm unlikely to spend any time on this.)

Perhaps we could experiment further with the configuration API. E.g., define a WriteLockStrategy trait to support this. That would become an additional generic parameter on Synchronizer, allowing us to skip the builder pattern. Still, I wouldn't make SingleWriter a default; IMO, toggling the feature flag shouldn't change the configuration of any Synchronizer without explicitly opting in.

Copy link

Choose a reason for hiding this comment

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

I'm not talking about the feature flag. "that it should become the default automatically when the write-lock feature is enabled?" - was my question.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That carries the risk of changing the behavior of existing code when the flag is enabled/disabled. Typically, feature flags enable/expose new APIs rather than controlling behavior directly. This is best as it should be possible to mix locking and non-locking Synchronizers even within a single project.

Choose a reason for hiding this comment

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

I guess I don't really see the distinction between enabling (and ostensibly using) new APIs rather than controlling behaviour. Point being is - what is the use for a Synchronizer if not locking (ignoring feature flags for the sake of argument)?

src/locks.rs Show resolved Hide resolved
src/locks.rs Show resolved Hide resolved
src/state.rs Show resolved Hide resolved
pub struct Synchronizer<
H: Hasher + Default = WyHash,
const N: usize = 1024,
const SD: u64 = 1_000_000_000,
WL = Disabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

To me it makes more sense to move WL right after H, so that it's easier to use synchronizer with N and SD defaults.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated. Note, this is a breaking change to any code specifying the N or SD parameters, so we'll need a major version bump when releasing.

src/locks.rs Outdated Show resolved Hide resolved
src/locks.rs Outdated Show resolved Hide resolved
src/state.rs Show resolved Hide resolved
@austinhartzheim
Copy link
Collaborator Author

austinhartzheim commented Oct 14, 2024

Updated to address feedback. Also, edited the PR description with new benchmarks and an updated description of the design.

Also, sealed the WriteLockStrategy trait to avoid committing to a specific interface and to avoid exposing internal lock mechanisms as part of the public interface.

These write locks prevent accidental misuse of `Synchronizer`s by
establishing an advisory lock on the state file using the `flock`
system call. Correctly configured writers will check this lock to
prevent duplicate writers.

By default, write locks are disabled and carry no additional cost
through use of generics. Unused functionality is optimized out by
the compiler during monomorphization.

Write locking is only supported on Unix-like platforms due to use
of the `flock` API. Other locking mechanisms and lock modes could
be supported in the future.
@austinhartzheim
Copy link
Collaborator Author

Rebased to simplify history. Bumped version to 2.0.0.

@austinhartzheim austinhartzheim merged commit 6b955b6 into cloudflare:main Oct 16, 2024
4 checks passed
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