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

Unnecessary buffer initialization in ByteQueue::push #1643

Open
sporksmith opened this issue Sep 15, 2021 · 2 comments
Open

Unnecessary buffer initialization in ByteQueue::push #1643

sporksmith opened this issue Sep 15, 2021 · 2 comments
Labels
Tag: Performance Related to improving shadow's run-time Type: Bug Error or flaw producing unexpected results

Comments

@sporksmith
Copy link
Contributor

sporksmith commented Sep 15, 2021

ByteQueue::push uses the standard library's Read::read_to_end. To avoid making uninitialized data available to the Read implementation, it currently initializes the buffer before passing it to read.

It looks like there's currently an internal / nightly API for marking a reader as promising not to read the buffer it's passed, in which case the initialization will be skipped.

In the meantime we may want to replace our usage of read_to_end with something that doesn't pre-initialize.

Example from a perf report, in a benchmark reading and writing 64 KB buffers via a pipe:

-   18.48%     0.26%             4  worker-0  shadow  [.] shadow_rs::utility::byte_queue::ByteQueue::push
   - 18.22% shadow_rs::utility::byte_queue::ByteQueue::push
      - 16.16% <std::io::Take<T> as std::io::Read>::read_to_end
         + 8.80% <shadow_rs::host::memory_manager::MemoryReaderCursor as std::io::Read>::read
           6.47% __memset_avx2_erms 

We spend almost as much time zeroing the buffer (__memset_avx2_erms) as copying the data (read)

@sporksmith sporksmith added Type: Bug Error or flaw producing unexpected results Tag: Performance Related to improving shadow's run-time labels Sep 15, 2021
@sporksmith
Copy link
Contributor Author

Btw it looks like the initializer method is unlikely to be stabilized. It's been superseded by the (merged, but unimplemented) ReadBuf RFC

@robgjansen
Copy link
Member

We decided this doesn't really fit in any milestone currently, and we are waiting for improvements in the Rust API before we can make progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tag: Performance Related to improving shadow's run-time Type: Bug Error or flaw producing unexpected results
Projects
None yet
Development

No branches or pull requests

2 participants