Stabilize debug_closure_helpers - #146099
coolreader18 wants to merge 5 commits into
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. |
… r=<try> Stabilize `debug_closure_helpers`
|
(I mean, we can still rebase, but it seems fine.) |
|
I just rebased it 2 weeks ago, to fix merge conflicts. |
rust-rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
There was a problem hiding this comment.
key and value have these panic sections. They should be updated here to mention key_with and value_with, and key_with and value_with should get the same thing.
Expect a minor conflict with #162825 if that merges first.
|
Wish I had noticed this earlier: we specify Nominating since this is already in FCP. @rustbot label +I-libs-nominated |
rustbot
added
the
I-libs-nominated
|
(Note: if you want to make that concern blocking, would need someone with checkbox privs to register it, but you can do that as a libs member.) |
This comment has been minimized.
This comment has been minimized.
rust-rfcbot
added
proposed-final-comment-period
|
team consensus is to change to using the impl per Trevor's comment - once that's done, we can restart fcp ^^ ty! |
clarfonthey
removed
the
I-libs-nominated
c47f842 to
38fed83
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. |
|
@rfcbot resolve impl-fnonce |
rust-rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
This comment has been minimized.
This comment has been minimized.
This branch has not been deployed
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