New syncing logic #63
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark for 4a65ecaClick to view benchmark
|
Pushkarm029
approved these changes
May 21, 2024
|
I think we have also discussed "smart rounding":
|
|
SystemTime records time at a nanosecond level depending on the operating system. The pub const fn as_millis(&self) -> u128 {
self.secs as u128 * MILLIS_PER_SEC as u128 + (self.nanos.0 / NANOS_PER_MILLI) as u128
} |
|
It's still possible theoretically:
I believe, that with millisecond precision, it becomes very rare case. But if we round read time down, then we eliminate this corner case completely. |
|
If we want to reduce the possibility of a race condition, we can use nano second resolution without any loss in terms of performance and increase in complexity. We are already is storing the SystemTime which is represented as a pub const fn as_nanos(&self) -> u128 {
self.secs as u128 * NANOS_PER_SEC as u128 + self.nanos.0 as u128
} |
Benchmark for 1d6e14fClick to view benchmark
|
|
@twitu yes, nanos are possible and are provided by the filesystem API, but iirc they are not real nanos and there are zeros at the end.. But why you don't want to adopt different rounding logic for reading timestamp? I think this would solve the issue for any precision. |
Benchmark for 02c5dafClick to view benchmark
|
|
I just noticed that because of the recent changes in the monoids PR. The semantics for the syncing logic has changed. We're now checking for timestamp equality, so rounding logic won't work. We can only increase the resolution of the timestamp. Also from the SystemTime docs, the resolution is OS dependent. Linux supports ns resolution and windows and mac atleast 100ns resolution. So the best way to cover all OS would be to use microsecond resolution. /// Compare the timestamp of the storage file
/// with the timestamp of the in-memory storage update
/// to determine if either of the two requires syncing.
fn needs_syncing(&self) -> Result<bool> {
match fs::metadata(&self.path) {
Ok(metadata) => {
let get_duration_since_epoch = |time: SystemTime| {
time.duration_since(SystemTime::UNIX_EPOCH)
.unwrap()
.as_secs()
};
let fs_modified =
get_duration_since_epoch(metadata.modified()?);
let self_modified = get_duration_since_epoch(self.modified);
Ok(fs_modified != self_modified) |
Benchmark for 2b24fe0Click to view benchmark
|
Benchmark for 4b74ce1Click to view benchmark
|
|
I just wanted to note that it's great the macOS CI is now failing, so we were able to reproduce the error. Using microseconds also results in the same error on macOS systems. |
Agree, this changes everything... Ready to merge as soon as build is fixed. |
twitu
changed the title
Benchmark for ef107deClick to view benchmark
|
|
A completely revamped syncing logic. This affects Needs a solid review. |
Benchmark for 1bd21fbClick to view benchmark
|
Benchmark for 897bb10Click to view benchmark
|
Benchmark for a925154Click to view benchmark
|
Benchmark for 559d655Click to view benchmark
|
Benchmark for f5a617aClick to view benchmark
|
Benchmark for 1ee6652Click to view benchmark
|
Could you write a minimal test to reproduce it? |
Benchmark for 66d9bddClick to view benchmark
|
Benchmark for 1a602a7Click to view benchmark
|
twitu
commented
Jun 11, 2024
Signed-off-by: Tarek <[email protected]>
It looks like this issue might be related to the file system's timestamp granularity. From what I've found, it seems to depend on the file system being used. For Linux:
I've pushed a commit that adds a one-second delay after each file write to fix the tests. This seems to be enough to fix the tests. I also reviewed the Rust file system documentation and added a call to I’m wondering if users might run into similar issues with |
Benchmark for aca4451Click to view benchmark
|
|
It's unfortunate that the OS does not update the metadata fast enough for it to be useful for us. However, we can work around it by setting the modified time our self. With this logic we can set the modified timestamp consistently and the tests pass without needing a sleep. fs::create_dir_all(parent_dir)?;
let mut file = File::create(&self.path)?;
file.write_all(serde_json::to_string_pretty(&self.data)?.as_bytes())?;
file.flush()?;
let new_timestamp = SystemTime::now();
file.set_modified(new_timestamp)?;
file.sync_all()?; |
Benchmark for d50b7a3Click to view benchmark
|
Benchmark for d86f413Click to view benchmark
|
tareknaser
approved these changes
Jun 14, 2024
tareknaser
reviewed
Jun 14, 2024
Co-authored-by: Tarek Elsayed <[email protected]>
Benchmark for 83e4f37Click to view benchmark
|
Benchmark for 7c12c54Click to view benchmark
|
Benchmark for a31c7a4Click to view benchmark
|
|
Good job @twitu @Pushkarm029 @tareknaser, this new logic looks very robust! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #37