Repository navigation
sync: build for wasm - #14769
sync: build for wasm#14769oech3 wants to merge 3 commits into
Conversation
|
GNU testsuite comparison: |
0a75721 to
4e15855
Compare
ffa6449 to
196bd97
Compare
|
It seems |
1e2c389 to
0dade4f
Compare
|
Permission denied on Windows... |
469590d to
d320767
Compare
15b97ad to
dfd1264
Compare
Merging this PR will improve performance by 39.29%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | three_39_bit_primes |
591.3 ms | 264 ms | ×2.2 |
| ⚡ | Simulation | five_38_bit_primes |
2.1 s | 1.9 s | +11.1% |
| ⚡ | Simulation | thirteen_39_bit_primes |
9.6 s | 8.9 s | +8.58% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing oech3:sync-wasi (3488fc4) with main (8b27533)2
Footnotes
-
54 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(a26a332) during the generation of this report, so 8b27533 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
|
OK to merge this? I'd like to port this for simpler workspace even feature is very limited. |
| #[cfg(not(any(target_os = "linux", target_os = "android", windows)))] | ||
| fn do_sync_with_regular_file<F>(files: &[String], op: F) -> UResult<()> | ||
| where | ||
| F: Fn(std::fs::File) -> std::io::Result<()>, | ||
| { | ||
| use uucore::error::FromIo; | ||
| for path in files { | ||
| let mut f = std::fs::OpenOptions::new(); | ||
| f.read(true); | ||
| // FIXME: sync_data() fails with permission denied even f.write(true) is used on Windows | ||
| #[cfg(windows)] | ||
| f.write(true); | ||
| op(f.open(path)?) | ||
| .map_err_context(|| translate!("sync-error-syncing-file", "file" => path.quote()))?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Before this PR, syncfs/fdatasync on macOS/BSD/wasm were not compiled at all, so sync -f/sync -d on those targets was a silent no-op. The existing Linux/Android platform::open_and_reset_nonblock deliberately opens with O_NONBLOCK and resets it, specifically to avoid hanging on FIFOs. The new fallback in do_sync_with_regular_file does a plain std::fs::OpenOptions::open() with no non-blocking handling, so sync -f /path/to/fifo on macOS/BSD/wasm can now hang indefinitely instead of being a no-op.
There was a problem hiding this comment.
Do I just need to set O_NONBLOCK?
| // FIXME: sync_data() fails with permission denied even f.write(true) is used on Windows | ||
| #[cfg(windows)] |
There was a problem hiding this comment.
I believe there is an error in this line as this check is never compiled to Windows.
There was a problem hiding this comment.
But I saw CI's failure.
There was a problem hiding this comment.
Probably before it was gated to not Windows.
There was a problem hiding this comment.
Even if the test is disabled, user see the error if the cfg was removed. Is it acceptible?
|
@eduardomourar I'm fine with this PR was replaced with your one if you have knowledge more than me. |
I don't want to hijack your PR. I am just reviewing it and helping us improve the code a little bit. |
08fafdb to
3488fc4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
I'd like to split this PR e.g. #15733 |
|
Oh wait! Does wasip* support NINBLOCK? I was too early to close this PR. |
Syncing a file seems possible on wasip at least for.