roachpb: refine Transaction proto cloning #35762
Merged
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
|
This change is |
tbg
approved these changes
Mar 25, 2019
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 30 of 30 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained
|
TIL of "Draft PRs". Sounds useful. |
|
bors r+ |
Build failed |
|
Looks like a build flake. One more time. bors r+ |
It turns out that it was easy enough to disallow interior mutability entirely within a Transaction and treat all of its references as immutable. Doing so allows us to only ever need to perform shallow clones of Transaction objects reducing the cost of a clone to a single, fixed-size allocation. Release note: None
Build failed (retrying...) |
Now that Transaction.Clone doesn't perform any deep copying, it can serve the role it should have had of simply cloning a Transaction to a new object on the heap. Callers who only need a value can use Transaction's full value semantics directly and simply copy it by value. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/fixTxnClone192
branch
from
1678583 to
c5dc833
Compare
Canceled |
|
Merge skew. Fixed. bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Mar 25, 2019
35762: roachpb: refine Transaction proto cloning r=nvanbenschoten a=nvanbenschoten Fixes #35803. This PR includes the final two commits from #35719. > By making Transaction.ObservedTimestamps immutable (which it almost already was), we can prohibit all interior mutability of references within Transaction, give it value semantics, and eliminate the distinction between "shallow" and "deep" object cloning. This reduces the cost of a clone to a single straightforward allocation and makes working with the object easier to think about. cc. @tbg Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
May 17, 2019
This debugging function was removed when we fixed cockroachdb#34241 and added back in when cockroachdb#35803 appeared because it was clear that we hadn't fully fixed the issue. It's been about 2 months since cockroachdb#35762 merged and we haven't seen any issues since, so this can now be removed. I don't think we meant to keep this in for the 19.1 release. We should backport this commit. Release note: None
craig bot
pushed a commit
that referenced
this pull request
May 17, 2019
37559: kv: remove withMarshalingDebugging function r=nvanbenschoten a=nvanbenschoten This debugging function was removed when we fixed #34241 and added back in when #35803 appeared because it was clear that we hadn't fully fixed the issue. It's been about 2 months since #35762 merged and we haven't seen any issues since, so this can now be removed. I don't think we meant to keep this in for the 19.1 release. We should backport this commit. Co-authored-by: Nathan VanBenschoten <[email protected]>
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.
Fixes #35803.
This PR includes the final two commits from #35719.
cc. @tbg