tracing: move ValueSet
construction out of closures
#987
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
ValueSet
s 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
ValueSet
constructionback 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
ValueSet
rework that will land intracing
0.2, where we could potentially generate significantly lesscode by virtue of constructing a
ValueSet
with a much simpler arrayliteral (no
FieldSet
iteration required).Fixes #954
Closes #986