Skip to content

Avoid allocating a digits array when writing decimal as numeric - #6729

Open
tamasprincz97-dot wants to merge 1 commit into
npgsql:mainfrom
tamasprincz97-dot:6728-decimal-write-allocation
Open

tamasprincz97-dot wants to merge 1 commit into
npgsql:mainfrom
tamasprincz97-dot:6728-decimal-write-allocation

Conversation

@tamasprincz97-dot

Copy link
Copy Markdown

Avoid allocating a digits array when writing decimal as numeric

Fixes #6728

DecimalNumericConverter<T>.Write stackalloc'd MaxDecimalNumericDigits (8) shorts. For a decimal's 3 uints, PgNumeric.Builder.Create reserves (3 * 10 + 3) / 4 + 1 = 9 digits, and falls back to new short[9] when the destination is smaller. So every non-null value written through this converter allocated 48 B, and the stack buffer was never used. That covers decimal, plus the byte/short/int/long/float/double to numeric mappings.

Changes:

  • PgNumeric.Builder: the bound expression moves into GetDestinationLength(bitsLength), which Create now uses. A new internal const int DecimalDestinationLength (9) uses the same expression, and AssertInvariants checks in debug builds that the two agree.
  • DecimalNumericConverter<T>.Write stackallocs DecimalDestinationLength.

MaxDecimalNumericDigits stays 8. That is the real maximum for a decimal, and ToDecimal and DecimalBasedMaxByteCount depend on it. A simpler MaxDecimalNumericDigits + 1 at the call site, or a tighter bound for decimals inside the builder, would also work. I preferred not to change the shared conversion code.

Tests (NumericTests):

  • New ReadWriteCases with 29 significant digits at scales 28, 27, 26, 5, 4, 3 and 1, positive and negative, so the existing read/write round-trip tests cover values that use all 8 numeric digits, with and without the scale-remainder digit.
  • Builder_from_decimal_uses_destination ([IssueLink] to Writing a decimal as numeric allocates a short[9] per value #6728) builds PgNumeric.Builder from a DecimalDestinationLength buffer. It checks that the digits land in that buffer (no array allocated) and that the value round-trips exactly. With the buffer set to 8, 68 of the 69 cases fail; only the value 0 passes, as it writes no digits. It tests the builder, not the converter's call site; the benchmark below covers that.

TypeHandlers.Numeric.Write (existing benchmark, MemoryDiagnoser), ShortRun, .NET 10.0.14, 4-core Xeon 2.1 GHz VM:

Value Allocated before Allocated after
0.0000000000000000000000000001 48 B -
0.0001 48 B -
1 48 B -
10000 48 B -
100000000 48 B -
10000000000000000 48 B -
10000000000000000000000000000 48 B -

All 15 benchmark values go from 48 B to 0 B. Mean times were 58-109 ns before and 49-99 ns after, but the ShortRun error bars overlap, so this PR claims only the allocation change.

Types.NumericTests and Types.NumericTypeTests pass on net10.0 in Debug and Release (372/372) against PostgreSQL 17.

For 10.0.x: the same change applies to hotfix/10.0.4, where the method is still WriteCore. git apply -3 merges the source files but leaves a conflict in NumericTests.cs.

Unrelated to #6509 (read-side ToDecimal bug).

DecimalNumericConverter.Write passed a stackalloc'd destination of
MaxDecimalNumericDigits (8) shorts to PgNumeric.Builder. The builder
reserves (3 * 10 + 3) / 4 + 1 = 9 digits for the 3 uints of a decimal,
so the destination was always too small and the builder allocated a
short[9] (48 B) for every non-null value written, including byte, short,
int, long, float and double values mapped to numeric.

Size the destination with a new DecimalDestinationLength constant, which
is derived from the same bound the builder uses (asserted in debug
builds).

Fixes npgsql#6728

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Writing a decimal as numeric allocates a short[9] per value

1 participant