Add benchmarks for impl Debug for str#124551
Merged
bors merged 1 commit intorust-lang:masterfrom May 10, 2024
Merged
Conversation
the8472
reviewed
Apr 30, 2024
Comment on lines
+22
to
+29
| assert_eq!(s.len(), 64); | ||
| assert_eq!(w.buf, expected); | ||
| assert_eq!(w.write_calls, expected_write_calls); |
Member
There was a problem hiding this comment.
Generally it's better to separate tests from benchmarks. That makes it easier to rewrite the benchmarks in case the optimizer gets too clever and optimizes things away. And it measures fewer things which should increase the SNR.
Right now this would also be benchmarking string comparison.
I know we have a few benchmarks which mix things, but they're not a good example to follow.
You can still write them in a single #[bench] function by running the asserts outside the benchmark loop.
Or you can declare the test strings as consts so they can be reused between tests and benches.
Contributor
Author
There was a problem hiding this comment.
You can still write them in a single #[bench] function by running the asserts outside the benchmark loop.
Or you can declare the test strings as consts so they can be reused between tests and benches.
I first tried to extract those to the tests directory, but then ended up leaving them together inside the #[bench] fn outside of the benchmark loop as you suggested.
IMO it makes sense to co-locate these, as especially counting / optimizing the write calls is done purely for performance reasons.
In order to inform future perf improvements and prevent regressions, lets add some benchmarks that stress `impl Debug for str`.
Member
|
Works for me! @bors r+ rollup |
Collaborator
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 10, 2024
Add benchmarks for `impl Debug for str` In order to inform future perf improvements and prevent regressions, lets add some benchmarks that stress `impl Debug for str`. --- As I am currently working on improving the perf in rust-lang#121150, its nice to have these benchmarks. Writing them, I also saw that escapes are written out one char at a time, even though other parts of the code are already optimizing that via `as_str`, which I intend to do as well as a followup improvement. r? `@cuviper` ☝🏻 as you were also assigned to rust-lang#121150, CC `@the8472` if you want to steal the review :-)
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 10, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#124551 (Add benchmarks for `impl Debug for str`) - rust-lang#124914 (Remove `#[macro_use] extern crate rustc middle` from numerous crates) - rust-lang#124915 (`rustc_target` cleanups) - rust-lang#124918 (Eliminate some `FIXME(lcnr)` comments) - rust-lang#124927 (opt-dist: use xz2 instead of xz crate) - rust-lang#124936 (analyse visitor: build proof tree in probe) - rust-lang#124943 (always use `GenericArgsRef`) Failed merges: - rust-lang#124955 (Use fewer origins when creating type variables.) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 10, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#124551 (Add benchmarks for `impl Debug for str`) - rust-lang#124915 (`rustc_target` cleanups) - rust-lang#124918 (Eliminate some `FIXME(lcnr)` comments) - rust-lang#124927 (opt-dist: use xz2 instead of xz crate) - rust-lang#124936 (analyse visitor: build proof tree in probe) - rust-lang#124943 (always use `GenericArgsRef`) - rust-lang#124955 (Use fewer origins when creating type variables.) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 10, 2024
Rollup merge of rust-lang#124551 - Swatinem:debug-str-bench, r=cuviper Add benchmarks for `impl Debug for str` In order to inform future perf improvements and prevent regressions, lets add some benchmarks that stress `impl Debug for str`. --- As I am currently working on improving the perf in rust-lang#121150, its nice to have these benchmarks. Writing them, I also saw that escapes are written out one char at a time, even though other parts of the code are already optimizing that via `as_str`, which I intend to do as well as a followup improvement. r? ``@cuviper`` ☝🏻 as you were also assigned to rust-lang#121150, CC ``@the8472`` if you want to steal the review :-)
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
May 17, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#124551 (Add benchmarks for `impl Debug for str`) - rust-lang#124915 (`rustc_target` cleanups) - rust-lang#124918 (Eliminate some `FIXME(lcnr)` comments) - rust-lang#124927 (opt-dist: use xz2 instead of xz crate) - rust-lang#124936 (analyse visitor: build proof tree in probe) - rust-lang#124943 (always use `GenericArgsRef`) - rust-lang#124955 (Use fewer origins when creating type variables.) r? `@ghost` `@rustbot` modify labels: rollup
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to inform future perf improvements and prevent regressions, lets add some benchmarks that stress
impl Debug for str.As I am currently working on improving the perf in #121150, its nice to have these benchmarks.
Writing them, I also saw that escapes are written out one char at a time, even though other parts of the code are already optimizing that via
as_str, which I intend to do as well as a followup improvement.r? @cuviper
☝🏻 as you were also assigned to #121150, CC @the8472 if you want to steal the review :-)