Repository navigation
Stabilize debug_closure_helpers - #146099
Conversation
rustbot
added
S-waiting-on-review
coolreader18
changed the title
debug_closure_helpers
Amanieu
added
I-libs-api-nominated
|
☔ The latest upstream changes (presumably #146636) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Once the FCP is complete, I think it might make sense to merge #145915 first and then merge this on top of that. |
| pub fn field_with<F>(&mut self, name: &str, value_fmt: F) -> &mut Self | ||
| where | ||
| F: FnOnce(&mut fmt::Formatter<'_>) -> fmt::Result, |
There was a problem hiding this comment.
Having the closure as a generic parameter (and not type-erasing it internally) leads to a lot of code bloat compared to the corresponding stable APIs that take &dyn Debug as value to format. According to cargo +nightly llvm-lines a simple program formatting a struct with two fields generates 694 lines of LLVM IR while the same program using field() generates 42 lines. So at least with the current implementation, these functions have an annoying downside compared to e.g. .field("foo", &fmt::from_fn(|f| ...)
I think this can be addressed after stabilization, without changing the signatures. But I wanted to flag it so the reviewer can think about it as well. It's not entirely obvious to me for the helpers that deal in FnOnce. I'm aware of one workaround (put the closure into an Option, then pass a &dyn FnMut that unwraps this option to call the original impl FnOnce) but it's not quite zero cost in several dimensions.
There was a problem hiding this comment.
Could this be solved by putting the main logic back into field(), and having field_with be an #[inline] wrapper around it?
I don't want to send in any PRs that might cause Git conflicts with the stabilization, but if the public API is good then I'd be happy to experiment with internal reorganization / optimization after both have landed.
There was a problem hiding this comment.
field takes a &dyn Debug. How do you invoke a FnOnce or FnMut from the &self argument of Debug::fmt?
There was a problem hiding this comment.
Cell<Option<F>> , then .replace(None).unwrap()? It doesn't need to survive more than one call.
There was a problem hiding this comment.
I think if field_with were changed to use dyn, it'd be important to ensure that it doesn't cause undue stack usage, since that was brought up as an issue with the existing functions in #117729 (comment)
There was a problem hiding this comment.
That report is in the context of a total stack size of 4 KiB. The DebugStruct::field function in my nightly toolchain's libcore.rlib allocates a very reasonable amount of stack space (add $0x48,%rsp). It then goes on to call other functions, but those seem to be functions shared with most of the formatting infrastructure. It's frankly a miracle that any variation of the code fits within a 4 KiB stack, and the cases that work probably depends on the code being duplicated and specialized a bit for the particular usage, which is fundamentally at odds with optimizing for code size (as core::fmt generally does). So I wouldn't give much weight to that report.
There was a problem hiding this comment.
@hanna-kruppe would you mind creating an issue for this? And/or a PR to change it? I think you're probably the best equipped to do so.
I'm ready to approve this but would like your confirmation that we have room to improve this within the current design.
There was a problem hiding this comment.
I can extract my above comment into an issue but I probably won't have time to try out a fix in the next 2-3 weeks. There's definitely room for improvement (e.g., the Cell<Option<F>>-via-&dyn Fn() approach discussed above). But without trying it out, I can't quantify how much improvement it is.
|
The stabilization PR for gentle ping @the8472 for review |
a51fcbe to
b1bde94
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #148544) made this pull request unmergeable. Please resolve the merge conflicts. |
b1bde94 to
1e39a61
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors retry |
|
@jmillikin: 🔑 Insufficient privileges: not in try users |
|
@the8472 gentle ping? |
|
r? libs-api |
|
☔ The latest upstream changes (presumably #149704) made this pull request unmergeable. Please resolve the merge conflicts. |
|
r? tgross35 Fyi you can The diff here LGTM, but I'd just like to make sure we have room to improve on what Hanna mentioned. |
fc4f85a to
aa70274
Compare
This comment has been minimized.
This comment has been minimized.
FYI: bors squash is a thing, if that's ever all that's blocking merge. |
|
Yeah if one commit made sense then I would, but I figured the author may still separate updates from stabilization (as they did) |
This comment has been minimized.
This comment has been minimized.
rust-rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
|
@rustbot author Just needs a rebase. |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
aa70274 to
384b61a
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r=tgross35 rollup Thank you! |
rust-bors
Bot
added
S-waiting-on-bors
14 participants
View all comments
Resolves #117729. The tracking issue still needs an FCP, but I'm hoping that creating a stabilization PR will prompt one.
r? libs-api