chore: Clean up merge logic for vectors of key-value structs. #1977
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
The issue is that the original code is not idiomatic rust, since it's directly indexing acc via acc[pos]. However, looking at merge_key_value_vecs, it already prefers the right value, so I _think_ we can just have the last two lines and get the same result. Including the refactored code as a comment so we can have a discussion on the PR, we may be doing the merge twice for cache line performance reasons.
github-actions
bot
added
the
type: chore
TheWizardTower
changed the title
…ailcallhq#1895) Co-authored-by: Tushar Mathur <tusharmath@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1977 +/- ##
==========================================
+ Coverage 82.24% 82.28% +0.04%
==========================================
Files 174 174
Lines 17763 17804 +41
==========================================
+ Hits 14609 14650 +41
Misses 3154 3154 ☔ View full report in Codecov by Sentry. |
tusharmath
reviewed
May 18, 2024
tusharmath
reviewed
May 18, 2024
tusharmath
approved these changes
May 22, 2024
|
LGTM! Thanks @TheWizardTower |
tusharmath
added a commit
that referenced
this pull request
May 22, 2024
Co-authored-by: Tushar Mathur <tusharmath@gmail.com> Co-authored-by: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Mehul Mathur <mathur.mehul01@gmail.com>
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.
The issue is that the original code is not idiomatic rust, since it's directly indexing acc via acc[pos]. However, looking at merge_key_value_vecs, it already prefers the right value, so I think we can just have the last two lines and get the same result.
Including the refactored code as a comment so we can have a discussion on the PR, we may be doing the merge twice for cache line performance reasons.
Summary:
I've rewritten the logic to merge the two vector maps to be more idiomatic Rust, and avoid a bare vector index. However, I think we can just call
merge_key_value_vecs()and achieve the same result, since that function biases towards the right values anyway.It's possible you all are doing it for optimization reasons, so I would appreciate review from folks who are more familiar with the codebase than I am.
Issue Reference(s):
Fixes #1701
Build & Testing:
cargo testsuccessfully../lint.sh --mode=fixto fix all linting issues raised by./lint.sh --mode=check.Checklist:
<type>(<optional scope>): <title>