Repository navigation
Conversation
The GetFieldValue<T> path (AddTypeMap(..., useGetFieldValue: true), DapperLib#1910) exists so that a value type can be read without going through GetValue and its box. It then boxed the value anyway: LoadReaderValueViaGetFieldValue stored a copy of every value into the object local that ThrowDataException reports in its "Error parsing column" message, and for a value type that copy is a box. So the path allocated exactly what the GetValue path allocates, only in Dapper instead of in the provider. Query<T> over 100k rows of six value-type columns (int, int, long, double, decimal, DateTime), bytes per row: DuckDB.NET GetValue 237 GetFieldValue 237 SqlClient GetValue 237 GetFieldValue 261 Keep the value unboxed instead. Each value type read through GetFieldValue<T> gets a local of its own type; the read stores the value there and records the column index. The catch block boxes the value into the object local only if that column is the one that failed, so the box happens on the error path and nowhere else. Reference types still go into the object local directly, and the GetValue path is unchanged. The same measurement with this change: DuckDB.NET GetFieldValue 85 SqlClient GetFieldValue 109 The 152 bytes per row that disappear are the six boxes. The 24 bytes SqlClient keeps are inside its own GetFieldValue<T> for one of the column types. This is the allocation DapperLib#1644 and DapperLib#1654 were after, limited to the opt-in path and without touching the message. The message is unchanged whenever a value was read: a failing setter or conversion still reports "Value=42 - Int32", and the test checks that against the GetValue path. The one difference is when GetFieldValue<T> itself throws. The message used to show the value of the previous column, because the object local was never cleared between columns; it now shows "n/a" and the cast error, as it already did for the first column. Re-reading the value from the reader in the catch block would have been simpler, but Dapper executes every query with CommandBehavior.SequentialAccess, and SqlClient and Npgsql refuse to read a consumed column again in that mode. Tests run against SQL Server: the full suite on net10.0, the new tests on net8.0 and net481 as well.
This branch has not been deployed
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.
The
GetFieldValue<T>path (AddTypeMap(..., useGetFieldValue: true), #1910) exists so that a value type can be read without going throughGetValueand its box. It then boxed the value anyway:LoadReaderValueViaGetFieldValuestored a copy of every value into the object local thatThrowDataExceptionreports in its "Error parsing column" message, and for a value type that copy is a box. So the path allocated exactly what theGetValuepath allocates, only in Dapper instead of in the provider.Query<T>over 100k rows of six value-type columns (int, int, long, double, decimal, DateTime), bytes per row, before this change:This PR keeps the value unboxed instead. Each value type read through
GetFieldValue<T>gets a local of its own type; the read stores the value there and records the column index. The catch block boxes the value into the object local only if that column is the one that failed, so the box happens on the error path and nowhere else. Reference types still go into the object local directly, and theGetValuepath is unchanged. The same measurement with the change:The 152 bytes per row that disappear are the six boxes. The 24 bytes SqlClient keeps are inside its own
GetFieldValue<T>for one of the column types. This is the allocation #1644 and #1654 were after, limited to the opt-in path and without touching the message.The message is unchanged whenever a value was read: a failing setter or conversion still reports
Value=42 - Int32, and the new test checks that against theGetValuepath. The one difference is whenGetFieldValue<T>itself throws. The message used to show the value of the previous column, because the object local was never cleared between columns; it now showsn/aand the cast error, as it already did for the first column.Re-reading the value from the reader in the catch block would have been simpler, but Dapper executes every query with
CommandBehavior.SequentialAccess, and SqlClient and Npgsql refuse to read a consumed column again in that mode.Tests run against SQL Server: the full suite on net10.0, the new tests on net8.0 and net481 as well.