Add async API #1
Conversation
|
Thank you very much! Support for async is very welcome and has indeed been on my todo list for quite a while. Disclaimer: I would suspect that in most cases the limitation is still parsing/formatting, and not blocking on I/O. To my understanding, other approaches to parallelisation, e.g. rayon, might be slightly more performant.
Apologies, I am running I am starting to review, but I am afraid it might take a bit longer. |
CardboardTurkey
force-pushed
the
cardboardturkey/async
branch
from
a5becbb
to
78d033a
Compare
Fixed!
True, some profiling would be interesting
Possibly, although rayon is most performant with parallelized iterators and I'm not sure that that would work so well in the case of HEP software. You could run an iterator over all events but then you have to hold all those events in memory at once - which would presumably be a lot.
Yeah this PR ended up being bigger than I expected. At least a lot of it is just the same changed applied multiple times 😅 |
src/lib.rs
Outdated
| //! | ||
| //! ```rust,no_run | ||
| //! # fn try_main() -> Result<(), Box<dyn std::error::Error>> { | ||
| //! // Read events from `events_in.hepmc2` and write them to `events_out.hepmc2` | ||
| //! use hepmc2::{Reader, Writer}; | ||
| //! | ||
| //! use std::io::BufReader; | ||
| //! use std::fs::File; | ||
| //! | ||
| //! let input = BufReader::new(File::open("events_in.hepmc2")?); | ||
| //! let in_events = Reader::from(input); | ||
| //! | ||
| //! let output = File::create("events_out.hepmc2")?; | ||
| //! let mut writer = Writer::try_from(output)?; | ||
| //! | ||
| //! for event in in_events { | ||
| //! let event = event?; | ||
| //! println!("Current cross section: {}", event.xs); | ||
| //! writer.write(&event)? | ||
| //! } | ||
| //! writer.finish()?; | ||
| //! # Ok(()) | ||
| //! # } | ||
| //! ``` | ||
| #![cfg_attr( | ||
| feature = "sync", | ||
| doc = r##" | ||
| ```rust,no_run | ||
| // Read events from `events_in.hepmc2` and write them to `events_out.hepmc2` | ||
| use hepmc2::{Reader, Writer}; | ||
|
|
||
| use std::io::BufReader; | ||
| use std::fs::File; | ||
|
|
||
| let input = BufReader::new(File::open("events_in.hepmc2")?); | ||
| let in_events = Reader::from(input); | ||
|
|
||
| let output = File::create("events_out.hepmc2")?; | ||
| let mut writer = Writer::try_from(output)?; | ||
|
|
||
| for event in in_events { | ||
| let event = event?; | ||
| println!("Current cross section: {}", event.xs); | ||
| writer.write(&event)? | ||
| } | ||
| writer.finish()?; | ||
| # Ok::<(), Box<dyn std::error::Error>>(()) | ||
| ```"## | ||
| )] | ||
| #![cfg_attr( | ||
| feature = "tokio", | ||
| doc = r##" | ||
| ```rust,no_run | ||
| # async fn try_main() -> Result<(), Box<dyn std::error::Error>> { | ||
| // Read events from `events_in.hepmc2` and write them to `events_out.hepmc2` | ||
| use hepmc2::{Reader, Writer}; | ||
|
|
||
| use tokio::io::BufReader; | ||
| use tokio::fs::File; | ||
|
|
||
| let input = BufReader::new(File::open("events_in.hepmc2").await?); | ||
| let mut in_events = Reader::from(input); | ||
|
|
||
| let output = File::create("events_out.hepmc2").await?; | ||
| let mut writer = Writer::try_from(output).await?; | ||
|
|
||
| while let Some(event) = in_events.next().await { | ||
| let event = event?; | ||
| println!("Current cross section: {}", event.xs); | ||
| writer.write(&event).await? | ||
| } | ||
| writer.finish().await?; | ||
| # Ok(()) | ||
| # } | ||
| # tokio_test::block_on(async {try_main().await.unwrap()}) | ||
| ```"## | ||
| )] | ||
|
|
There was a problem hiding this comment.
I think it would make sense to have both the sync and the async example code in the documentation, regardless of which features are enabled. We can then put a #[cfg] inside the examples to make the actual compilation depend on the features.
At the moment, the doc here and the readme are the same - the readme is in fact generated with cargo readme. I would like to keep it that way, i.e. we would copy the new text in the readme over to the doc here.
There was a problem hiding this comment.
Done. Found a way to do this with cfg_attr outside of the examples, which I think I prefer.
I'm afraid though the readme no longer matches cargo-readme - it can't handle doc tests gated behind features: webern/cargo-readme#54
src/writer.rs
Outdated
There was a problem hiding this comment.
cargo criterion shows a huge regression (~50%) in the sync write performance after commit 6c56562. Maybe we need wrapper macros that depending on the features forward to either write!/writeln! in sync mode or the format!/write_all combination in async mode.
There was a problem hiding this comment.
Now that is concerning! I've been distracted by FOSDEM lately, but will have a think about this now. Wrapper macros might be the way to go if I can find an implementation that isn't too complex
There was a problem hiding this comment.
Hi! So I think I've fixed this now, could you confirm?
I followed your suggestion of adding a wrapper macro. I'm happy with this approach but there are two small annoyances:
- attribute macros like
cfgcan only be applied to statements (not expressions), this forced me to include the?operator within the macro, - I couldn't find a nice way of implementing a
writeln!wrapper because there's no async method for writing a line.
It will of course depend very much on the hardware, but in another project I got a huge speedup with synchronous reading of the raw ASCII event records and parallelised parsing through a rayon scope. Again, that was for reading from a local hard disk; event output and network filesystem I/O will be a different beast again. |
CardboardTurkey
force-pushed
the
cardboardturkey/async
branch
4 times, most recently
from
cbea9ca
to
3cd7210
Compare
CardboardTurkey
force-pushed
the
cardboardturkey/async
branch
from
3cd7210
to
faca720
Compare
Apply a `cargo fmt` (with line limit of 80) run and remove redundant redefinition of `random_state` to sate clippy. Signed-off-by: Kiran Ostrolenk <[email protected]>
As of 1.34.0 you don't need to embed doc tests in dummy functions just to use the question mark operator. See https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#using--in-doc-tests for more details. Signed-off-by: Kiran Ostrolenk <[email protected]>
This adds a `tokio` feature that allows for async writes using `tokio::io::AsyncWriteExt`. It also adds a `sync` feature, enabled by default, that exposes the original sync API. Code duplication between the two APIs is avoided using a combination of [maybe-async] and the `write_bound` attribute macro defined in `hepmc2-macros`. The former automatically removes `async`/`await` keywords for the sync API and the latter is used to choose the correct trait bounds based on which API is enabled. [maybe-async]: https://crates.io/crates/maybe-async A `maybe_async` macro is also introduced to ensure sync writes can continue using the `std::write` macro and so avoid the performance cost of allocating intermediate formatted strings. Signed-off-by: Kiran Ostrolenk <[email protected]>
This follows a very similar logic to the write API. Signed-off-by: Kiran Ostrolenk <[email protected]>
This adds a section to the README and some doc tests. This also gates the sync and async doc tests behind their respective features. Signed-off-by: Kiran Ostrolenk <[email protected]>
CardboardTurkey
force-pushed
the
cardboardturkey/async
branch
from
faca720
to
8b5ba48
Compare
There was a problem hiding this comment.
Excellent, the performance regression is gone. Thanks very much!
- As you note, it may be a good idea to implement
futures::stream::Streamor whichever equivalent gets stabilised at some point. - async benchmarks would be neat.
- I am trying to think of a more descriptive name for the new
maybe_write!.
None of these are major and at least the first two should be separate pull requests anyway.
@a-maier this is a really useful crate - thanks for building it! I've got a contribution here to add an async API to the library.
Motivation
Asynchronous programming allows programs to continue doing work when waiting on IO calls, rather than being blocked by them. In the case of
hepmc2it would allow, for example, downstream event generators to continue calculating MEs while waiting for an event to be written to a HepMC file. Given the size of HepMC files that physicists like to generate this could result in sizable performance improvements!Changes
The first two commits here are minor pieces of house keeping - the meat of the PR begins after that. Please see the commit messages for some guidance on the changes.
In summary, the async API has exactly the same function signatures as the sync one (
Future's not withstanding). I managed to avoid almost all code duplication using a combination of maybe_async and a couple of new macros I added to the library. The latter are placed in a new workspace crate calledhepcm2-macros(as is standard practice when writing proc macros).maybe-asyncIn case you're unfamiliar with it, I can briefly describe the
maybe-asynccrate. It provides an attribute macromaybe_async::maybe_asyncthat should be applied to blocks of async code. In the case where the featuremaybe-async/is_syncis enabled, the macro removes all occurrences of theasyncandawaitkeywords in the block, effectively turning it into sync code. This is how I'm able to use the compiler to generate the sync API from the async one, and so avoiding code duplication.