Improve Select and Retrieve performance#14
Open
devin-ai-integration[bot] wants to merge 3 commits intomasterfrom
Open
Improve Select and Retrieve performance#14devin-ai-integration[bot] wants to merge 3 commits intomasterfrom
devin-ai-integration[bot] wants to merge 3 commits intomasterfrom
Conversation
- Hoist WrappedRecord allocation out of per-row loop in ConvertDataReader
- Hoist WrappedReader allocation out of per-row loop in ConvertDataReaderMultiResult
- Fix Hash.Compute thread safety: replace static mutable fields with local variables
- Replace new Param[] {} with Array.Empty<Param>() to avoid allocations
- Optimize GuessOperationType to use indexed loop instead of LINQ Any()
- Optimize GetQueryKey: reduce LINQ overhead with manual StringBuilder iteration
- Remove redundant Log.Capture calls in cache hit/miss paths
Co-Authored-By: Max Stepanskiy <mstepanskiy@asicentral.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…d cannot be shared across rows Co-Authored-By: Max Stepanskiy <mstepanskiy@asicentral.com>
- Cache primary key column names per type to avoid per-row LINQ in CreateIdentity - Pre-allocate args array and cache map/typeCount before per-row loop - Cache config.AutoTypeCoercion before loops to avoid repeated property access - Bypass Map dispatch in ConvertDataReader: call FastIndexerMapper directly, avoiding per-row IsIndexer type checks and null checks - Pre-cache Mapper delegates for secondary types in multi-map queries, avoiding per-row Tuple allocation and ConcurrentDictionary lookup - Pre-resolve Mapper delegate per result set in ConvertDataReaderMultiResult - Cache types[resultIndex] in local variable to avoid repeated list indexer access - Replace LINQ ToList with array allocation for reflectedTypes in multi-result path - Pre-size HashSet with FieldCount capacity in GetColumns (netstandard2.1+/net8.0) Co-Authored-By: Max Stepanskiy <mstepanskiy@asicentral.com>
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
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.
Summary
Reduces per-call and per-row allocations and overhead in the Select/Retrieve hot paths.
Allocation & overhead reductions (initial pass)
Hash.Computethread safety: Replaced static mutable fields (_a,_b,_c) with local variables passed byref. The old implementation was not thread-safe — concurrent callers would corrupt shared state, producing incorrect hash values and cache misses.new Param[] {}→Array.Empty<Param>(): Avoids allocating a throwaway empty array on every cached Retrieve call (both sync and async paths).GuessOperationType: indexedforloop instead ofoperation.Any(char.IsWhiteSpace): Avoids delegate allocation per call.GetQueryKeyoptimization: Changed parameter fromIEnumerable<Param>toIList<Param>, replaced LINQ chain (OrderBy/Select/ToDelimitedString) with a manualStringBuilderloop, and skips sorting when there's only one parameter.Log.Capturecalls in cache hit/miss paths to reduce string interpolation overhead.Deeper per-row optimizations (latest revision)
Mapdispatch inConvertDataReader: The genericMap<TSource, TResult>method performsMappingFactory.IsIndexer(3 type checks) + additionalispattern matching on every row, even though the source is alwaysIDataRecord. Now callsFastIndexerMapper<IDataRecord, T>.Map/FastIndexerMapperWithTypeCoercion<IDataRecord, T>.Mapdirectly.Mapper.PropertyMapperdelegates for multi-map secondary types: Avoids per-rowTuple<Type,Type,bool,bool>allocation +ConcurrentDictionary.GetOrAddlookup inMapper.CreateDelegatefor each secondary type.ConvertDataReaderMultiResult: Same optimization for multi-result queries.PrimaryKeyColumnCacheavoids per-row LINQ (Where/Select/OrderBy/ToArray) inCreateIdentity.argsarray before per-row loop: Avoidsnew object[types.Count]allocation on every row in multi-map scenarios.config.AutoTypeCoercionandtypes[resultIndex]in locals: Avoids repeated property access and list indexer lookups per row.ToList()with array allocation forreflectedTypesin multi-result path.HashSetwithFieldCountcapacity inGetColumns(netstandard2.1+/net8.0 only due to API availability).All 230 unit tests pass.
Review & Testing Checklist for Human
FastIndexerMapperdispatch is correct — The code now bypassesMap<TSource, TResult>and callsFastIndexerMapper<IDataRecord, T>.Mapdirectly. Verify this is semantically equivalent for all entity types (interfaces, concrete classes, with/without type coercion).Mapper.CreateDelegate(typeof(IDataRecord), types[i], true, autoTypeCoercion)is called once before the loop. The original code went throughMap(object, Type, bool)which didIsIndexer+GetIndexerType. Confirm the pre-cached delegate produces identical results.Log.CaptureEndbalance — The diff removesLog.CaptureBegincalls but keeps someLog.CaptureEndcalls on the cache-hit return paths. This may cause mismatched begin/end in the logging stack.GetQueryKeyoutput format changed slightly (no trailing "/" when params are empty). Existing in-memory L1 cache keys will no longer match after upgrade, causing a one-time cache miss on first call.Nemo.Benchmarkproject against a database to measure actual throughput improvement on Select/Retrieve paths (e.g.,dotnet run -c Releaseintests/Nemo.Benchmark/).Notes