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

Tracking Issue for Read::is_read_vectored/Write::is_write_vectored. #69941

Open
1 task
sfackler opened this issue Mar 12, 2020 · 7 comments
Open
1 task

Tracking Issue for Read::is_read_vectored/Write::is_write_vectored. #69941

sfackler opened this issue Mar 12, 2020 · 7 comments
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@sfackler
Copy link
Member

Unresolved Questions

  • What's the right naming for these methods?

Implementation history

@sfackler sfackler added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 12, 2020
@JohnTitor JohnTitor added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Mar 12, 2020
@KodrAus KodrAus added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` I-nominated Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@workingjubilee
Copy link
Member

Is there a situation under which this would not be better off as an associated constant?

@sfackler
Copy link
Member Author

Those are not compatible with trait objects.

@bhgomes
Copy link
Contributor

bhgomes commented Nov 21, 2021

Those are not compatible with trait objects.

Is this a design choice, or just something that has not been implemented yet?

@sfackler
Copy link
Member Author

That is inherent to how constants work.

@m-ou-se
Copy link
Member

m-ou-se commented Dec 28, 2022

Is there a situation under which this would not be better off as an associated constant?

#105917 is an interesting case to think about: io::Chain. When chaining one stream that is write vectored with another one that isn't, io::Chain::is_write_vectored could make its return value depend on whether it's currently in the first or second stream, I suppose. (Not saying that it should. But we should probably document what is expected.)

@xiaoyawei
Copy link

Is there a timeline to stabilize this feature? Knowing whether a socket supports vectored I/O or not gives applications a bunch of optimization opportunities :)

@cosmicexplorer
Copy link

As one potential argument for the acceptance of io::Write::is_write_vectored(), I would like to note that Tokio has stabilized tokio::io::AsyncWrite::is_write_vectored(). However, tokio::io::AsyncRead does not expose any analogue for is_read_vectored(), which may be a potential argument against io::Read::is_read_vectored(). I'm guessing that vectored reads are less likely to improve performance in async code as opposed to largely-synchronous local filesystem i/o though, so tokio's decision to avoid vectored reads is likely not pertinent to the general utility of vectored reads.

As @xiaoyawei noted, it would be especially nice to be able to detect support for vectored writes on stable. In an experimental branch of the zip crate, I demonstrated writing zip headers in a single call using vectored writes (https://github.com/cosmicexplorer/zip/blob/f755697eff4f342c842e2c76ae2bb387515de56b/src/spec.rs#L214-L241):

    pub async fn write_async<T: io::AsyncWrite>(&self, mut writer: Pin<&mut T>) -> ZipResult<()> {
        let block: [u8; 22] = unsafe {
            mem::transmute(CentralDirectoryEndBuffer {
                magic: CENTRAL_DIRECTORY_END_SIGNATURE,
                disk_number: self.disk_number,
                disk_with_central_directory: self.disk_with_central_directory,
                number_of_files_on_this_disk: self.number_of_files_on_this_disk,
                number_of_files: self.number_of_files,
                central_directory_size: self.central_directory_size,
                central_directory_offset: self.central_directory_offset,
                zip_file_comment_length: self.zip_file_comment.len() as u16,
            })
        };


        if writer.is_write_vectored() {
            /* TODO: zero-copy!! */
            let block = IoSlice::new(&block);
            let comment = IoSlice::new(&self.zip_file_comment);
            writer.write_vectored(&[block, comment]).await?;
        } else {
            /* If no special vector write support, just perform two separate writes. */
            writer.write_all(&block).await?;
            writer.write_all(&self.zip_file_comment).await?;
        }


        Ok(())
    }
}

I suppose it's true that I could have done this without checking is_write_vectored(), but if vectored write support isn't guaranteed, I don't want to risk a non-atomic write (not sure if this really matters, I guess it just makes me uncomfortable).

However, I'm now also realizing that the above code is probably incorrect, as I don't check whether tokio's async write_vectored() writes all of my data, so I'm probably looking for #70436/write_all_vectored() to be stabilized as well (and I can see there is a legitimate discussion required before that can be stabilized). I will conclude this note by proposing that especially in the absence of a stabilized write_all_vectored(), it becomes even more important to avoid complex vectorized writes unless necessary, and I think this makes is_{read,write}_vectored() even more useful to stabilize first.

Regarding @m-ou-se's comment on io::Chain behavior (#69941 (comment)):

#105917 is an interesting case to think about: io::Chain. When chaining one stream that is write vectored with another one that isn't, io::Chain::is_write_vectored could make its return value depend on whether it's currently in the first or second stream, I suppose. (Not saying that it should. But we should probably document what is expected.)

@workingjubilee's proposal at https://github.com/rust-lang/rust/pull/105917/files#r1278629167 to make io::Chain only report is_read_vectored() if both first and second supported vectored reads seems like the most intuitive response to this question.

Thanks so much for reading, I really appreciate your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants