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

optimize perfermance of fd_read/fd_write #8303

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

liutao-liu
Copy link
Contributor

@liutao-liu liutao-liu commented Apr 5, 2024

Compared to native(C/C++), fd-read can only reach 20% of native. I will divide the performance hotspots into the following two categories:,

  1. Wasi implementation of fd-read(approximately 30% of the hotspots), such as the cost of redundant memory copies, the cost of inserting and deleting HostInputStream into WasiView, and the cost of accessing guest memory(merged PR);
  2. Wasm program call host function(approximately 40% of the hotspots): I am not very familiar with this mechanism yet, and currently I cannot understand the reasons behind the hotspots here;

This PR is mainly optimized for the first type of performance hotspot:

  1. Avoid unnecessary copying;
  2. Avoid inserting and deleting HostInputStream intoWasiView;

The perfermance hotspots of fd_write is the same fd_read:

let stream = self.append_via_stream(fd).map_err(|e| {

Vec::from(chunk),

@liutao-liu liutao-liu requested a review from a team as a code owner April 5, 2024 06:08
@liutao-liu liutao-liu requested review from alexcrichton and removed request for a team April 5, 2024 06:08
@liutao-liu liutao-liu force-pushed the opt_read_write branch 3 times, most recently from ce78858 to adee567 Compare April 5, 2024 06:42
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Apr 5, 2024
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I've left some notes below, but I'll additionally point out that this is specifically ignoring the blocking_mode now. Previously that was "respected" to a certain degree, but now it's being ignored. I believe that's ok because that's what the previous implementation in wasi-common did anyway as OSes tend to ignore nonblocking on files. That being said the current state of the code is a bit confusing where the blocking_mode is sometimes used and sometimes isn't.

I mostly wanted to point this out in case anyone runs into this in the future. I'll follow up with a PR after this one to clean this up. You don't have to clean it up here yourself @liutao-liu

crates/wasi/src/preview1.rs Outdated Show resolved Hide resolved
crates/wasi/src/preview1.rs Outdated Show resolved Hide resolved
crates/wasi/src/preview1.rs Outdated Show resolved Hide resolved
@liutao-liu liutao-liu force-pushed the opt_read_write branch 2 times, most recently from d978cc4 to cf7dfd6 Compare April 6, 2024 04:23
@liutao-liu
Copy link
Contributor Author

@alexcrichton Your review comments have been completed,this pr can be merged.

@alexcrichton alexcrichton added this pull request to the merge queue Apr 11, 2024
Merged via the queue into bytecodealliance:main with commit 7cc63de Apr 11, 2024
19 checks passed
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Apr 11, 2024
* Explicitly document why `blocking_mode` is being ignored.
* Don't write-in-a-loop to doing a partial write and then returning an
  error.
* Spawn a background read/write when the embedding cannot block the
  current thread.
* Don't panic on overflows for addition.

Follow-ups to bytecodealliance#8303
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Apr 11, 2024
* Explicitly document why `blocking_mode` is being ignored.
* Don't write-in-a-loop to doing a partial write and then returning an
  error.
* Spawn a background read/write when the embedding cannot block the
  current thread.
* Don't panic on overflows for addition.
* Fix panic on reads with shared memory

Follow-ups to bytecodealliance#8303
github-merge-queue bot pushed a commit that referenced this pull request Apr 12, 2024
* Explicitly document why `blocking_mode` is being ignored.
* Don't write-in-a-loop to doing a partial write and then returning an
  error.
* Spawn a background read/write when the embedding cannot block the
  current thread.
* Don't panic on overflows for addition.
* Fix panic on reads with shared memory

Follow-ups to #8303
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants