refactor: remove expensive clones from verify_consistency_of_operations; convert API to slice #285
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
…ns; convert API to slice There was never any need for this function to take in a &Vec; we should never do that. Instead take in a slice. Additionally, we refactored some of the loop logic to use .take instead of a check on each iteration to see if we were at the end. We also combined two duplicate for loops into 1. Finally, we significantly refactor the return type to avoid a lot of very expensive cloning. Previously, we were doing something on the magnitude of 9 clones (of expensive underlying objects including vectors) per input GroveDbOp. All of these have been removed. We instead return references to these values instead of clones of them. This does mean that the lifetime of the return value is tied to the underlying data the slice refers to; but especially as I cannot even find an instance where we use these return values, this feels like a very fair trade off.
PastaPastaPasta
requested review from
QuantumExplorer and
fominok
as code owners
|
Hahaha, the original code was 🙈. |
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.
Issue being fixed or feature implemented
Remove unneeded cloning, and use slice
What was done?
There was never any need for this function to take in a &Vec; we should never do that. Instead take in a slice.
Additionally, we refactored some of the loop logic to use .take instead of a check on each iteration to see if we were at the end. We also combined two duplicate for loops into 1.
Finally, we significantly refactor the return type to avoid a lot of very expensive cloning. Previously, we were doing something on the magnitude of 9 clones (of expensive underlying objects including vectors) per input GroveDbOp. All of these have been removed. We instead return references to these values instead of clones of them. This does mean that the lifetime of the return value is tied to the underlying data the slice refers to; but especially as I cannot even find an instance where we use these return values, this feels like a very fair trade off.
How Has This Been Tested?
running
cargo tlocallyBreaking Changes
Well... It seems in actually usage it is none; however, if someone is using these return values currently; this probably could be breaking. Also it may require converting some stuff to a slice. Please let me know if I should mark this PR title with a
!Checklist:
For repository code-owners and collaborators only