Use non-generic inner function for pointer formatting #91266
Merged
Conversation
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
|
r? @yaahc (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors r+ |
|
📌 Commit 37c8f25 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 27, 2021
…askrgr Rollup of 6 pull requests Successful merges: - rust-lang#83791 (Weaken guarantee around advancing underlying iterators in zip) - rust-lang#90995 (Document non-guarantees for Hash) - rust-lang#91057 (Expand `available_parallelism` docs in anticipation of cgroup quota support) - rust-lang#91062 (rustdoc: Consolidate static-file replacement mechanism) - rust-lang#91208 (Account for incorrect `where T::Assoc = Ty` bound) - rust-lang#91266 (Use non-generic inner function for pointer formatting) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
|
This sounds great! And thank you for putting a comment explaining the inner function. One minor point: because this landed in a rollup, if it has an effect on the rustc-perf benchmark suite that effect might be hard to disentangle from the effects of other PRs in that rollup. Therefore, for PRs that affect compiler performance, it's best if a CI perf run can be performed before merging, by giving bors the |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously, despite the implementation being type-unaware,
fmt::Pointer's implementation for*const Tin monomorphized. This affects:fmt::Debugfor*const Tfmt::Debugfor*mut Tfmt::Pointerfor*const Tfmt::Pointerfor*mut TAnd since the implementation is non-trivial, this results in a large amount of LLVM bitcode being generated. For example, with a large bindgen project with Debug implementations enabled, it will generate a lot of calls to
fmt::Debug for *const T, which in turn will perform codegen for a copy of this function for every type.For example, in a real-world bindgen'd header I've been testing with (4,189,245 lines of bindgen Rust with layout tests disabled) the difference between a slightly old nightly (
rustc 1.58.0-nightly (e249ce6b2 2021-10-30)) and this PR:Nightly (Click to Expand)
This PR (Click to Expand)
Output generated using
cargo llvm-linesversion 0.4.12.Summary of differences:
*const T as fmt::PointerLLVM linesnightlyThis results in a pretty noticeable as the majority of rustc's time is spent in either codegen or LLVM, in this case, and is significantly improved by disabling derives for
fmt::Debug, as it prevents generating all this LLVM IR to be handled.Here's a run time comparison with nightly on the same codebase (commit 454cc5f built from source vs 37c8f25 from my PR built from source):
nightly (Click to Expand)
This PR (Click to Expand)
(6.34% decrease in runtime, results are consistent across multiple runs)