-
Notifications
You must be signed in to change notification settings - Fork 645
delete_by_col_eq: optimize with smallvec for 1 row case #547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
benchmarks please |
Benchmark resultsBenchmark ReportLegend:
All throughputs are single-threaded. Empty transaction
Single-row insertions
Multi-row insertions
Full table iterate
Find unique key
Filter
Serialize
Module: invoke with large arguments
Module: print bulk
Remaining benchmarks
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable, and I appreciate the comment explaining why SmallVec is useful here. We don't seem to have benchmarks for deletes, so I can't say whether this is a performance win or how much of one it is, but intuition says avoiding an allocation in the common path is a win.
I suspect as well but would like to include the "...at the expense of branches" in the train of thought. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just needs cargo fmt
6e61261 to
0542027
Compare
Description of Changes
I noticed that
delete_by_col_eqis mostly called bydelete_by_fieldwhich hasdebug_assert!(count <= 1);.This suggests that we do an unnecessary allocation in most cases.
We can get rid of that by using a
SmallVec<[RowId; 1]>so that in thecount = 1case, we don't need to allocate.API and ABI breaking changes
None.
Expected complexity level and risk
1