MOD-9985 Refactoring of C Code for the RSSortingVector Port#6312
Merged
MOD-9985 Refactoring of C Code for the RSSortingVector Port#6312
Conversation
2 tasks
DarthB
force-pushed
the
rp-sortvec-crefactor
branch
from
95c19b3 to
46f0fd3
Compare
JoanFM
reviewed
Jun 13, 2025
src/value.h
Outdated
| } | ||
| } | ||
|
|
||
| void RSValue_IncrRef_Rust(RSValue* v); |
Collaborator
There was a problem hiding this comment.
I think it may be better not to name them Rust, but call the static one with some _ underscore to imply "private"?
Contributor
Author
There was a problem hiding this comment.
Good point, having two variants of a function raises questions.
Instead of a _ prefix, a _inl postfix is more descriptive, what do you think?
Addressed in 365cd88
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6312 +/- ##
==========================================
- Coverage 88.90% 88.84% -0.07%
==========================================
Files 241 242 +1
Lines 40793 40796 +3
Branches 3435 3435
==========================================
- Hits 36266 36244 -22
- Misses 4491 4516 +25
Partials 36 36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/value.h
Outdated
Comment on lines
152
to
164
| // Use RSValue_IncrRef_inl for performance in C; RSValue_IncrRef for FFI | ||
| static inline RSValue *RSValue_IncrRef_inl(RSValue *v) { | ||
| __atomic_fetch_add(&v->refcount, 1, __ATOMIC_RELAXED); | ||
| return v; | ||
| } | ||
| void RSValue_IncrRef(RSValue* v); | ||
|
|
||
| #define RSValue_Decref(v) \ | ||
| if (!__atomic_sub_fetch(&(v)->refcount, 1, __ATOMIC_RELAXED)) { \ | ||
| RSValue_Free(v); \ | ||
| // Use RSValue_DecrRef_inl for performance in C; RSValue_IncrRef for FFI | ||
| static inline void RSValue_DecrRef_inl(RSValue* v) { | ||
| if (__atomic_sub_fetch(&(v)->refcount, 1, __ATOMIC_RELAXED) == 0) { | ||
| RSValue_Free(v); | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
I would keep the original name for these two functions, using a more verbose name for the FFI ones, since those will only be used through bindings in Rust.
Collaborator
There was a problem hiding this comment.
More generally, do we truly need two definitions? The C compiler may inline these functions even if not marked as static inline. A benchmark run might clarify if the change is required.
Contributor
Author
There was a problem hiding this comment.
You mean the ones started by the action:run_benchmark label?
Contributor
Author
There was a problem hiding this comment.
src/aggregate/aggregate_exec.c
Outdated
Comment on lines
51
to
53
| size_t len = RSSortingVector_Length(r->rowdata.sv); | ||
| if ((kk->flags & RLOOKUP_F_SVSRC) && (r->rowdata.sv && len > kk->svidx)) { | ||
| return RSSortingVector_Get(r->rowdata.sv, kk->svidx); |
Collaborator
There was a problem hiding this comment.
I would move the length access after the null check on rowdata.sv.
Comment on lines
55
to
61
| static inline size_t RSSortingVector_Length(const RSSortingVector* vec) { | ||
| if (vec) { | ||
| return vec->len; | ||
| } else { | ||
| return 0; | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Should we have this defaulting behaviour for the null case?
DarthB
force-pushed
the
rp-sortvec-crefactor
branch
from
be743d2 to
cc2e2c4
Compare
Contributor
Automated performance analysis summaryThis comment was automatically generated given there is performance data available. In summary:
You can check a comparison in detail via the grafana link Comparison between master and rp-sortvec-crefactor.Time Period from 30 days ago. (environment used: oss-standalone)
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Describe the changes in the pull request
RSSortingVectorSortingVector_RdbLoadfunctionRSValueto be used by Rust code too.Extracted from: #6311
Mark if applicable