-
Notifications
You must be signed in to change notification settings - Fork 3
Optimize PackedFieldAccessor.GetValue<>()
#439
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
base: master-servicetitan
Are you sure you want to change the base?
Conversation
| var columnIndexes = provider.ColumnIndexes; | ||
|
|
||
| var n = columnIndexes.Count; | ||
| using PooledArray<SqlColumn> pooledArray = new(n); |
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.
PooledArray has own drawbacks and not a silver bullet. Need to bench before apply.
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.
I consider ArrayPool<> to be always faster than temporary array allocation/collection.
This was the intention of the ArrayPool<> design.
No need to benchmark every appearance of it
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 is questionable concideration, moreover It was not true when we tried it 3-4 years ago in ST.
I couldn't approve this PR without bench.
botinko
left a comment
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.
PooledArray is slower.
BenchmarkDotNet v0.15.8, Windows 11 (10.0.26200.7462/25H2/2025Update/HudsonValley2) IterationCount=5 WarmupCount=3
|
The benchmark ignores GC time. that is why it looks as |
Remove
isNullableparameter. It can be calculated at compile time and compile can apply some optimizations knowing it.Also:
PackedFieldAccessor.Rankfield as unusedPackedFieldAccessor.Indexto byte to reduce this class sizePooedArrayinVisitSelect()