tracing: move ValueSet construction out of closures
#987
Conversation
This file contains 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
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky
approved these changes
Sep 25, 2020
hawkw
added a commit
that referenced
this pull request
Sep 28, 2020
Fixed - Incorrect inlining of `Span::new`, `Span::new_root`, and `Span::new_child_of`, which could result in `dispatcher::get_default` being inlined at the callsite ([#994]) - Regression where using a struct field as a span or event field when other fields on that struct are borrowed mutably would fail to compile ([#987]) Changed - Updated `tracing-core` to 0.1.17 ([#992]) Added - `Instrument` trait and `Instrumented` type for attaching a `Span` to a `Future` ([#808]) - `Copy` implementations for `Level` and `LevelFilter` ([#992]) - Multiple documentation fixes and improvements ([#964], [#980], [#981]) Thanks to @nagisa, and new contributors @securityinsanity, @froydnj, @jyn514 and @TaKO8Ki for contributing to this release! [#994]: #994 [#992]: #992 [#987]: #987 [#980]: #980 [#981]: #981 [#964]: #964 [#808]: #808 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Sep 28, 2020
### Fixed - Incorrect inlining of `Span::new`, `Span::new_root`, and `Span::new_child_of`, which could result in `dispatcher::get_default` being inlined at the callsite ([#994]) - Regression where using a struct field as a span or event field when other fields on that struct are borrowed mutably would fail to compile ([#987]) ### Changed - Updated `tracing-core` to 0.1.17 ([#992]) ### Added - `Instrument` trait and `Instrumented` type for attaching a `Span` to a `Future` ([#808]) - `Copy` implementations for `Level` and `LevelFilter` ([#992]) - Multiple documentation fixes and improvements ([#964], [#980], [#981]) Thanks to @nagisa, and new contributors @securityinsanity, @froydnj, @jyn514 and @TaKO8Ki for contributing to this release! [#994]: #994 [#992]: #992 [#987]: #987 [#980]: #980 [#981]: #981 [#964]: #964 [#808]: #808 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
dvdplm
added a commit
to dvdplm/tracing
that referenced
this pull request
Oct 5, 2020
* master: tracing: Instrument std::future::Future (tokio-rs#808) tracing: move `ValueSet` construction out of closures (tokio-rs#987)
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.
Motivation
In PR #943, the construction of
ValueSets for events and spans wasmoved out of the code expanded at the callsite and into a closure, in
order to reduce the amount of assembly generated in functions containing
tracing macros. However, this introduced an accidental breaking change
for some dependent crates. Borrowing values inside a closure meant that
when a field of a struct, array, or tuple was used as a field value, the
closure must borrow the entire struct, array, or tuple, rather than
the individual field. This broke code where another unrelated field of
that struct, array, or tuple would then be mutably borrowed or moved
elsewhere.
Solution
This branch fixes the breaking change by moving
ValueSetconstructionback out of a closure and into the code expanded at the callsite. This
does regress the amount of assembly generated a bit: a function
containing a single
event!macro generates 32 instructions in releasemode on master, and after this change, it generates 83 instructions.
However, this is better than reverting PR #943 entirely, which generates
103 instructions for the same function. This change allows us to
continue to benefit from some of the changes made in #943, although we
no longer can benefit from the most significant one.
Rather than trying to further optimize the macro expanded code now, I
think we should wait for the
ValueSetrework that will land intracing0.2, where we could potentially generate significantly lesscode by virtue of constructing a
ValueSetwith a much simpler arrayliteral (no
FieldSetiteration required).Fixes #954
Closes #986