On Tue, May 28, 2024 at 02:48:42PM +0800, Zhao Liu wrote: > Hi Stefan, > > On Mon, May 27, 2024 at 03:59:44PM -0400, Stefan Hajnoczi wrote: > > Date: Mon, 27 May 2024 15:59:44 -0400 > > From: Stefan Hajnoczi <stefa...@redhat.com> > > Subject: Re: [RFC 0/6] scripts: Rewrite simpletrace printer in Rust > > > > On Mon, May 27, 2024 at 04:14:15PM +0800, Zhao Liu wrote: > > > Hi maintainers and list, > > > > > > This RFC series attempts to re-implement simpletrace.py with Rust, which > > > is the 1st task of Paolo's GSoC 2024 proposal. > > > > > > There are two motivations for this work: > > > 1. This is an open chance to discuss how to integrate Rust into QEMU. > > > 2. Rust delivers faster parsing. > > > > > > > > > Introduction > > > ============ > > > > > > Code framework > > > -------------- > > > > > > I choose "cargo" to organize the code, because the current > > > implementation depends on external crates (Rust's library), such as > > > "backtrace" for getting frameinfo, "clap" for parsing the cli, "rex" for > > > regular matching, and so on. (Meson's support for external crates is > > > still incomplete. [2]) > > > > > > The simpletrace-rust created in this series is not yet integrated into > > > the QEMU compilation chain, so it can only be compiled independently, e.g. > > > under ./scripts/simpletrace/, compile it be: > > > > > > cargo build --release > > > > Please make sure it's built by .gitlab-ci.d/ so that the continuous > > integration system prevents bitrot. You can add a job that runs the > > cargo build. > > Thanks! I'll do this. > > > > > > > The code tree for the entire simpletrace-rust is as follows: > > > > > > $ script/simpletrace-rust . > > > . > > > ├── Cargo.toml > > > └── src > > > └── main.rs // The simpletrace logic (similar to simpletrace.py). > > > └── trace.rs // The Argument and Event abstraction (refer to > > > // tracetool/__init__.py). > > > > > > My question about meson v.s. cargo, I put it at the end of the cover > > > letter (the section "Opens on Rust Support"). > > > > > > The following two sections are lessons I've learned from this Rust > > > practice. > > > > > > > > > Performance > > > ----------- > > > > > > I did the performance comparison using the rust-simpletrace prototype with > > > the python one: > > > > > > * On the i7-10700 CPU @ 2.90GHz machine, parsing and outputting a 35M > > > trace binary file for 10 times on each item: > > > > > > AVE (ms) Rust v.s. Python > > > Rust (stdout) 12687.16 114.46% > > > Python (stdout) 14521.85 > > > > > > Rust (file) 1422.44 264.99% > > > Python (file) 3769.37 > > > > > > - The "stdout" lines represent output to the screen. > > > - The "file" lines represent output to a file (via "> file"). > > > > > > This Rust version contains some optimizations (including print, regular > > > matching, etc.), but there should be plenty of room for optimization. > > > > > > The current performance bottleneck is the reading binary trace file, > > > since I am parsing headers and event payloads one after the other, so > > > that the IO read overhead accounts for 33%, which can be further > > > optimized in the future. > > > > Performance will become more important when large amounts of TCG data is > > captured, as described in the project idea: > > https://wiki.qemu.org/Internships/ProjectIdeas/TCGBinaryTracing > > > > While I can't think of a time in the past where simpletrace.py's > > performance bothered me, improving performance is still welcome. Just > > don't spend too much time on performance (and making the code more > > complex) unless there is a real need. > > Yes, I agree that it shouldn't be over-optimized. > > The logic in the current Rust version is pretty much a carbon copy of > the Python version, without additional complex logic introduced, but the > improvements in x2.6 were obtained by optimizing IO: > > * reading the event configuration file, where I called the buffered > interface, > * and the output formatted trace log, which I output all via std_out.lock() > followed by write_all(). > > So, just the simple tweak of the interfaces brings much benefits. :-) > > > > Security > > > -------- > > > > > > This is an example. > > > > > > Rust is very strict about type-checking, and it found timestamp reversal > > > issue in simpletrace-rust [3] (sorry, haven't gotten around to digging > > > deeper with more time)...in this RFC, I workingaround it by allowing > > > negative values. And the python version, just silently covered this > > > issue up. > > > > > > Opens on Rust Support > > > ===================== > > > > > > Meson v.s. Cargo > > > ---------------- > > > > > > The first question is whether all Rust code (including under scripts) > > > must be integrated into meson? > > > > > > If so, because of [2] then I have to discard the external crates and > > > build some more Rust wheels of my own to replace the previous external > > > crates. > > > > > > For the main part of the QEMU code, I think the answer must be Yes, but > > > for the tools in the scripts directory, would it be possible to allow > > > the use of cargo to build small tools/program for flexibility and > > > migrate to meson later (as meson's support for rust becomes more > > > mature)? > > > > I have not seen a satisfying way to natively build Rust code using > > meson. I remember reading about a tool that converts Cargo.toml files to > > meson wrap files or something similar. That still doesn't feel great > > because upstream works with Cargo and duplicating build information in > > meson is a drag. > > > > Calling cargo from meson is not ideal either, but it works and avoids > > duplicating build information. This is the approach I would use for now > > unless someone can point to an example of native Rust support in meson > > that is clean. > > > > Here is how libblkio calls cargo from meson: > > https://gitlab.com/libblkio/libblkio/-/blob/main/src/meson.build > > https://gitlab.com/libblkio/libblkio/-/blob/main/src/cargo-build.sh > > Many thanks! This is a good example and I'll try to build similarly to > it. > > > > > > > > > > External crates > > > --------------- > > > > > > This is an additional question that naturally follows from the above > > > question, do we have requirements for Rust's external crate? Is only std > > > allowed? > > > > There is no policy. My suggestion: > > > > If you need a third-party crate then it's okay to use it, but try to > > minimize dependencies. Avoid adding dependening for niceties that are > > not strictly needed. Third-party crates are a burden for package > > maintainers and anyone building from source. They increase the risk that > > the code will fail to build. They can also be a security risk. > > Thanks for the suggestion, that's clear to me, I'll try to control the > third party dependencies. > > > > > > > Welcome your feedback! > > > > It would be easier to give feedback if you implement some examples of > > TCG binary tracing before rewriting simpletrace.py. It's unclear to me > > why simpletrace needs to be rewritten at this point. If you are > > extending the simpletrace file format in ways that are not suitable for > > Python or can demonstrate that Python performance is a problem, then > > focussing on a Rust simpletrace implementation is more convincing. > > > > Could you use simpletrace.py to develop TCG binary tracing first? > > Yes, I can. :-) > > Rewriting in Rust does sound duplicative, but I wonder if this could be > viewed as an open opportunity to add Rust support for QEMU, looking at > the scripts directory to start exploring Rust support/build would be > a relatively easy place to start. > > I think the exploration of Rust's build of the simpletrace tool under > scripts parts can help with subsequent work on supporting Rust in other > QEMU core parts. > > From this point, may I ask your opinion? > > Maybe later, Rust-simpletrace and python-simpletrace can differ, e.g. > the former goes for performance and the latter for scalability.
Rewriting an existing, maintained component without buy-in from the maintainers is risky. Mads is the maintainer of simpletrace.py and I am the overall tracing maintainer. While the performance improvement is nice, I'm a skeptical about the need for this and wonder whether thought was put into how simpletrace should evolve. There are disadvantages to maintaining multiple implementations: - File format changes need to be coordinated across implementations to prevent compatibility issues. In other words, changing the trace-events format becomes harder and discourages future work. - Multiple implementations makes life harder for users because they need to decide between implementations and understand the trade-offs. - There is more maintenance overall. I think we should have a single simpletrace implementation to avoid these issues. The Python implementation is more convenient for user-written trace analysis scripts. The Rust implementation has better performance (although I'm not aware of efforts to improve the Python implementation's performance, so who knows). I'm ambivalent about why a reimplementation is necessary. What I would like to see first is the TCG binary tracing functionality. Find the limits of the Python simpletrace implementation and then it will be clear whether a Rust reimplementation makes sense. If Mads agrees, I am happy with a Rust reimplementation, but please demonstrate why a reimplementation is necessary first. Stefan
signature.asc
Description: PGP signature
