chore: revert #943 #986
Closed
chore: revert #943 #986
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
hawkw
added a commit
that referenced
this pull request
Sep 25, 2020
## Motivation In PR #943, the construction of `ValueSet`s for events and spans was moved 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` construction back 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 release mode 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 in `tracing` 0.2, where we could potentially generate significantly less code by virtue of constructing a `ValueSet` with a much simpler array literal (no `FieldSet` iteration required). Fixes #954 Closes #986 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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.
This commit appears to break Tower.