Skip to content

Commit

Permalink
Switching flush for sync_data
Browse files Browse the repository at this point in the history
A person joined Discord reporting they traced Nebari and didn't see any
explicit fsync operations. I was incredulous, but discovered that
File::flush() is a no-op.

Switching to sync_data on my Linux development machine caused no
performance changes.

On Mac, however, that's a different story. The comments in the SQLite
benchmarks explain why. For the Roots type, where multiple files need to
be flushed, an F_BARRIERFSYNC on each file, then an F_FULLFSYNC might be
a faster way to flush multiple trees on Mac OS. However, any changes
other than this simple change will involve extra dependencies or unsafe
code, so I'm living with this downgrade in performance on Mac for now.

Both Nebari and SQLite suffer on Mac OS with F_FULLFSYNC, but Nebari
suffers *more* than SQLite. I am hopeful that there might be performance
left on the table that could improve Nebari on all platforms.
  • Loading branch information
ecton committed May 3, 2022
1 parent 9e284e6 commit 3718ffb
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 1 deletion.
17 changes: 17 additions & 0 deletions benchmarks/benches/blobs/sqlite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,23 @@ impl SimpleBench for InsertBlobs {
// better performance. See:
// https://www.sqlite.org/pragma.html#pragma_synchronous
sqlite.pragma_update(None, "synchronous", &"NORMAL")?;
// Mac OS does not implement fsync() with the same guarantees as Linux.
// From Apple's guide "Reducing Disk Writes"
// (https://developer.apple.com/documentation/xcode/reducing-disk-writes#Minimize-Explicit-Storage-Synchronization):
//
// > Only use F_FULLFSYNC when your app requires a strong expectation of
// > data persistence. Note that F_FULLFSYNC represents a best-effort
// > guarantee that iOS writes data to the disk, but data can still be
// > lost in the case of sudden power loss.
//
// Rust's implementation of `File::sync_data` calls `fcntl` with
// `F_FULLFSYNC` on Mac OS, which means Nebari is performing the
// strongest guarantees that Apple provides that bits are fully
// persisted to disk before reporting a succesful result. SQLite does
// not enable this by default, and instead opts for better performance
// over a best-attempt at ACID-compliance.
#[cfg(target_os = "mac_os")]
sqlite.pragma_update(None, "fullfsync", &"true")?;
sqlite.execute("create table blobs (id integer primary key, data blob)", [])?;
Ok(Self {
sqlite,
Expand Down
17 changes: 17 additions & 0 deletions benchmarks/benches/logs/sqlite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,23 @@ impl SimpleBench for InsertLogs {
// better performance. See:
// https://www.sqlite.org/pragma.html#pragma_synchronous
sqlite.pragma_update(None, "synchronous", &"NORMAL")?;
// Mac OS does not implement fsync() with the same guarantees as Linux.
// From Apple's guide "Reducing Disk Writes"
// (https://developer.apple.com/documentation/xcode/reducing-disk-writes#Minimize-Explicit-Storage-Synchronization):
//
// > Only use F_FULLFSYNC when your app requires a strong expectation of
// > data persistence. Note that F_FULLFSYNC represents a best-effort
// > guarantee that iOS writes data to the disk, but data can still be
// > lost in the case of sudden power loss.
//
// Rust's implementation of `File::sync_data` calls `fcntl` with
// `F_FULLFSYNC` on Mac OS, which means Nebari is performing the
// strongest guarantees that Apple provides that bits are fully
// persisted to disk before reporting a succesful result. SQLite does
// not enable this by default, and instead opts for better performance
// over a best-attempt at ACID-compliance.
#[cfg(target_os = "mac_os")]
sqlite.pragma_update(None, "fullfsync", &"true")?;
sqlite.execute(
"create table logs (id integer primary key, timestamp integer, message text)",
[],
Expand Down
2 changes: 1 addition & 1 deletion nebari/src/io/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl Write for StdFile {

#[cfg_attr(feature = "tracing", tracing::instrument(skip(self)))]
fn flush(&mut self) -> std::io::Result<()> {
self.file.flush()
self.file.sync_all()
}
}

Expand Down

0 comments on commit 3718ffb

Please sign in to comment.