From c47477dc515a9710e8df415306757b2512b3c1d6 Mon Sep 17 00:00:00 2001 From: Giorgi Dalakishvili Date: Fri, 30 Oct 2026 21:09:36 +0400 Subject: [PATCH] Stop boxing values read via GetFieldValue for the error message The GetFieldValue path (AddTypeMap(..., useGetFieldValue: true), #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 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 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 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 test checks that against the GetValue path. The one difference is when GetFieldValue 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. --- Dapper/SqlMapper.cs | 99 +++++++++++++++++++++++---- tests/Dapper.Tests/DataReaderTests.cs | 70 ++++++++++++++++++- 2 files changed, 155 insertions(+), 14 deletions(-) diff --git a/Dapper/SqlMapper.cs b/Dapper/SqlMapper.cs index 8f3956cab..f613a364e 100644 --- a/Dapper/SqlMapper.cs +++ b/Dapper/SqlMapper.cs @@ -3472,7 +3472,7 @@ private static void GenerateValueTupleDeserializer(Type valueTupleType, DbDataRe il, startBound + i, ref stringEnumLocal, - valueCopyLocal: null, + valueCopyLocals: null, reader.GetFieldType(startBound + i), targetType, out var isDbNullLabel, out bool popWhenNull); @@ -3517,6 +3517,7 @@ private static void GenerateDeserializerFromMap(Type type, DbDataReader reader, var returnValueLocal = il.DeclareLocal(type); il.Emit(OpCodes.Ldc_I4_0); il.Emit(OpCodes.Stloc, currentIndexDiagnosticLocal); + var valueCopyDiagnosticLocals = new ValueCopyDiagnosticLocals(il); var names = Enumerable.Range(startBound, length).Select(i => reader.GetName(i)).ToArray(); @@ -3610,7 +3611,6 @@ private static void GenerateDeserializerFromMap(Type type, DbDataReader reader, bool first = true; var allDone = il.DefineLabel(); var stringEnumLocal = (LocalBuilder?)null; - var valueCopyDiagnosticLocal = il.DeclareLocal(typeof(object)); bool applyNullSetting = Settings.ApplyNullValues; foreach (var item in members) { @@ -3625,7 +3625,7 @@ private static void GenerateDeserializerFromMap(Type type, DbDataReader reader, EmitInt32(il, index); il.Emit(OpCodes.Stloc, currentIndexDiagnosticLocal); - LoadReaderValueOrBranchToDBNullLabel(il, index, ref stringEnumLocal, valueCopyDiagnosticLocal, reader.GetFieldType(index), memberType, out var isDbNullLabel, out bool popWhenNull); + LoadReaderValueOrBranchToDBNullLabel(il, index, ref stringEnumLocal, valueCopyDiagnosticLocals, reader.GetFieldType(index), memberType, out var isDbNullLabel, out bool popWhenNull); if (specializedConstructor is null) { @@ -3711,9 +3711,10 @@ private static void GenerateDeserializerFromMap(Type type, DbDataReader reader, } il.MarkLabel(allDone); il.BeginCatchBlock(typeof(Exception)); // stack is Exception + valueCopyDiagnosticLocals.EmitBoxTypedValueOfCurrentColumn(il, currentIndexDiagnosticLocal); // stack is Exception il.Emit(OpCodes.Ldloc, currentIndexDiagnosticLocal); // stack is Exception, index il.Emit(OpCodes.Ldarg_0); // stack is Exception, index, reader - il.Emit(OpCodes.Ldloc, valueCopyDiagnosticLocal); // stack is Exception, index, reader, value + il.Emit(OpCodes.Ldloc, valueCopyDiagnosticLocals.ObjectLocal); // stack is Exception, index, reader, value il.EmitCall(OpCodes.Call, typeof(SqlMapper).GetMethod(nameof(SqlMapper.ThrowDataException))!, null); il.EndExceptionBlock(); @@ -3740,7 +3741,75 @@ private static void LoadDefaultValue(ILGenerator il, Type type) } } - private static void LoadReaderValueViaGetFieldValue(ILGenerator il, int index, Type memberType, LocalBuilder? valueCopyLocal, Label isDbNullLabel, out bool popWhenNull) + /// + /// The locals that keep the value of the column being read, so that can show it + /// when a column fails. A value read via is already an object and goes into + /// . A value read via stays unboxed in a + /// local of its own type and is boxed in the catch block, only if its column is the one that failed, so that the + /// GetFieldValue path does not allocate per value. + /// + private sealed class ValueCopyDiagnosticLocals + { + public LocalBuilder ObjectLocal { get; } + private readonly LocalBuilder typedValueIndexLocal; // the column whose value is in a typed local, or -1 + private readonly Dictionary typedLocals = []; + private readonly List> typedColumns = []; + + public ValueCopyDiagnosticLocals(ILGenerator il) + { + ObjectLocal = il.DeclareLocal(typeof(object)); + typedValueIndexLocal = il.DeclareLocal(typeof(int)); + EmitInt32(il, -1); + il.Emit(OpCodes.Stloc, typedValueIndexLocal); + } + + /// + /// Stores the value on top of the stack in the typed local for , leaving it on the stack. + /// + public void EmitStoreTypedValue(ILGenerator il, int index, Type type) + { + if (!typedLocals.TryGetValue(type, out var local)) + { + local = il.DeclareLocal(type); + typedLocals.Add(type, local); + } + il.Emit(OpCodes.Dup); // stack is now [...][T][T] + il.Emit(OpCodes.Stloc, local); // stack is now [...][T] + EmitInt32(il, index); + il.Emit(OpCodes.Stloc, typedValueIndexLocal); + typedColumns.Add(new KeyValuePair(index, type)); + } + + /// + /// For the catch block: if the value of the failing column is in a typed local, boxes it into + /// . Leaves the stack as it was. + /// + public void EmitBoxTypedValueOfCurrentColumn(ILGenerator il, LocalBuilder currentIndexLocal) + { + if (typedColumns.Count == 0) return; + + var done = il.DefineLabel(); + // a typed value belongs to the failing column only if it was stored while reading that column + il.Emit(OpCodes.Ldloc, typedValueIndexLocal); + il.Emit(OpCodes.Ldloc, currentIndexLocal); + il.Emit(OpCodes.Bne_Un, done); + foreach (var column in typedColumns) + { + var next = il.DefineLabel(); + il.Emit(OpCodes.Ldloc, typedValueIndexLocal); + EmitInt32(il, column.Key); + il.Emit(OpCodes.Bne_Un, next); + il.Emit(OpCodes.Ldloc, typedLocals[column.Value]); + il.Emit(OpCodes.Box, column.Value); + il.Emit(OpCodes.Stloc, ObjectLocal); + il.Emit(OpCodes.Br, done); + il.MarkLabel(next); + } + il.MarkLabel(done); + } + } + + private static void LoadReaderValueViaGetFieldValue(ILGenerator il, int index, Type memberType, ValueCopyDiagnosticLocals? valueCopyLocals, Label isDbNullLabel, out bool popWhenNull) { popWhenNull = false; var underlyingType = Nullable.GetUnderlyingType(memberType) ?? memberType; @@ -3755,14 +3824,18 @@ private static void LoadReaderValueViaGetFieldValue(ILGenerator il, int index, T il.Emit(OpCodes.Ldarg_0); // stack is now [...][reader] EmitInt32(il, index); // stack is now [...][reader][index] il.Emit(OpCodes.Callvirt, getFieldValueT.MakeGenericMethod(underlyingType)); // stack is now [...][T] - if (valueCopyLocal is not null) + if (valueCopyLocals is not null) { - il.Emit(OpCodes.Dup); // stack is now [...][T][T] if (underlyingType.IsValueType) { - il.Emit(OpCodes.Box, underlyingType); // stack is now [...][T][value-as-object] + // keep the value unboxed; the catch block boxes it only if this column fails + valueCopyLocals.EmitStoreTypedValue(il, index, underlyingType); // stack is now [...][T] + } + else + { + il.Emit(OpCodes.Dup); // stack is now [...][T][T] + il.Emit(OpCodes.Stloc, valueCopyLocals.ObjectLocal); // stack is now [...][T] } - il.Emit(OpCodes.Stloc, valueCopyLocal); // stack is now [...][T] } if (underlyingType != memberType) { @@ -3771,12 +3844,12 @@ private static void LoadReaderValueViaGetFieldValue(ILGenerator il, int index, T } } - private static void LoadReaderValueOrBranchToDBNullLabel(ILGenerator il, int index, ref LocalBuilder? stringEnumLocal, LocalBuilder? valueCopyLocal, Type colType, Type memberType, out Label isDbNullLabel, out bool popWhenNull) + private static void LoadReaderValueOrBranchToDBNullLabel(ILGenerator il, int index, ref LocalBuilder? stringEnumLocal, ValueCopyDiagnosticLocals? valueCopyLocals, Type colType, Type memberType, out Label isDbNullLabel, out bool popWhenNull) { isDbNullLabel = il.DefineLabel(); if (UseGetFieldValue(memberType) && !IsDateTimeFamilyConversion(colType, Nullable.GetUnderlyingType(memberType) ?? memberType)) { - LoadReaderValueViaGetFieldValue(il, index, memberType, valueCopyLocal, isDbNullLabel, out popWhenNull); + LoadReaderValueViaGetFieldValue(il, index, memberType, valueCopyLocals, isDbNullLabel, out popWhenNull); return; } @@ -3786,10 +3859,10 @@ private static void LoadReaderValueOrBranchToDBNullLabel(ILGenerator il, int ind // default impl: use GetValue il.Emit(OpCodes.Callvirt, getItem); // stack is now [...][value-as-object] - if (valueCopyLocal is not null) + if (valueCopyLocals is not null) { il.Emit(OpCodes.Dup); // stack is now [...][value-as-object][value-as-object] - il.Emit(OpCodes.Stloc, valueCopyLocal); // stack is now [...][value-as-object] + il.Emit(OpCodes.Stloc, valueCopyLocals.ObjectLocal); // stack is now [...][value-as-object] } if (memberType == typeof(char) || memberType == typeof(char?)) diff --git a/tests/Dapper.Tests/DataReaderTests.cs b/tests/Dapper.Tests/DataReaderTests.cs index f98bfabed..bd36e8c1d 100644 --- a/tests/Dapper.Tests/DataReaderTests.cs +++ b/tests/Dapper.Tests/DataReaderTests.cs @@ -1,4 +1,6 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; +using System.Data; using System.Data.Common; using System.Linq; using Xunit; @@ -283,6 +285,72 @@ union all Assert.Equal("qwe", bar.HazNameIdObject.Name); } + [Fact] + public void GetFieldValue_ErrorMessageShowsTheValue() + { + // the message names the value that was read whichever reader method read it; on the + // GetFieldValue path the value is kept unboxed and boxed only when its column fails + SqlMapper.AddTypeMap(typeof(long), DbType.Int64, useGetFieldValue: true); + SqlMapper.AddTypeMap(typeof(int), DbType.Int32, useGetFieldValue: true); + try + { + var ex = Assert.Throws(() => connection.Query("select cast(7 as bigint) as Id, 42 as Value -- GetFieldValue")); + Assert.Equal("Error parsing column 1 (Value=42 - Int32)", ex.Message); + Assert.IsType(ex.InnerException); + + ex = Assert.Throws(() => connection.Query("select cast(7 as bigint) as Id, 42 as Value -- GetFieldValue")); + Assert.Equal("Error parsing column 1 (Value=42 - Int32)", ex.Message); + Assert.IsType(ex.InnerException); + } + finally + { + SqlMapper.AddTypeMap(typeof(long), DbType.Int64, useGetFieldValue: false); + SqlMapper.AddTypeMap(typeof(int), DbType.Int32, useGetFieldValue: false); + } + + // the same message as the GetValue path gives; a separate type, because deserializers are cached + // per type and column shape, so the same type would reuse the deserializer compiled above + var viaGetValue = Assert.Throws(() => connection.Query("select cast(7 as bigint) as Id, 42 as Value -- GetValue")); + Assert.Equal("Error parsing column 1 (Value=42 - Int32)", viaGetValue.Message); + } + + [Fact] + public void GetFieldValue_ErrorMessageWhenTheReadItselfFails() + { + // when GetFieldValue throws there is no value to show, and the message must not show the previous column's + SqlMapper.AddTypeMap(typeof(long), DbType.Int64, useGetFieldValue: true); + SqlMapper.AddTypeMap(typeof(int), DbType.Int32, useGetFieldValue: true); + try + { + var ex = Assert.Throws(() => connection.Query("select cast(7 as bigint) as Id, 'abc' as Value -- GetFieldValue")); + Assert.StartsWith("Error parsing column 1 (Value=n/a - ", ex.Message); + Assert.NotNull(ex.InnerException); + } + finally + { + SqlMapper.AddTypeMap(typeof(long), DbType.Int64, useGetFieldValue: false); + SqlMapper.AddTypeMap(typeof(int), DbType.Int32, useGetFieldValue: false); + } + } + + private class HazThrowingSetter + { + public long Id { get; set; } + public int Value { get => 0; set => throw new InvalidOperationException("Value cannot be set"); } + } + + private class HazThrowingNullableSetter + { + public long Id { get; set; } + public int? Value { get => null; set => throw new InvalidOperationException("Value cannot be set"); } + } + + private class HazThrowingSetterViaGetValue + { + public long Id { get; set; } + public int Value { get => 0; set => throw new InvalidOperationException("Value cannot be set"); } + } + private abstract class Discriminated_BaseType { public abstract int Type { get; }