Skip to content

perf: replace string concatenation with arithmetic in DecimalType.serialize (hundreds of ns, x1.3-2.6 speedup) - #764

Draft
mykaul wants to merge 1 commit into
scylladb:masterfrom
mykaul:perf/decimal-serialize-arithmetic
Draft

perf: replace string concatenation with arithmetic in DecimalType.serialize (hundreds of ns, x1.3-2.6 speedup)#764
mykaul wants to merge 1 commit into
scylladb:masterfrom
mykaul:perf/decimal-serialize-arithmetic

Conversation

@mykaul

@mykaul mykaul commented Mar 25, 2026

Copy link
Copy Markdown

Summary

  • Replace int(''.join([str(d) for d in digits])) with a multiply-and-add loop in DecimalType.serialize()
  • Eliminates per-digit str() allocations and the intermediate joined string

Details

Decimal.as_tuple() returns digits as a tuple of ints (0-9). The previous code converted each digit to a string, joined them, and parsed the result back to int. The new code uses simple arithmetic: n = n * 10 + digit.

Before

unscaled = int(''.join([str(digit) for digit in digits]))

After

unscaled = 0
for digit in digits:
    unscaled = unscaled * 10 + digit

Benchmark results (isolated digit-to-integer conversion)

Digit count String join (ns) Arithmetic (ns) Speedup
3 228 84 2.70x
20 1079 681 1.58x
31 1502 1125 1.34x

Benchmark script included at benchmarks/decimal_serialize.py.

Testing

  • All 651 existing unit tests pass (16 pre-existing skips)
  • Correctness verified via round-trip serialize/deserialize for small, medium, and large Decimal values

@mykaul
mykaul marked this pull request as draft March 25, 2026 20:32
@mykaul mykaul changed the title perf: replace string concatenation with arithmetic in DecimalType.serialize perf: replace string concatenation with arithmetic in DecimalType.serialize (hundreds of ns, x1.3-2.6 speedup) Apr 7, 2026
Copilot AI review requested due to automatic review settings July 29, 2026 20:18
@mykaul
mykaul force-pushed the perf/decimal-serialize-arithmetic branch from a2f4e20 to a08da22 Compare July 29, 2026 20:18
@mykaul

mykaul commented Jul 29, 2026

Copy link
Copy Markdown
Author

Equivalence verification

Rebased onto current master (fast-forwarded cleanly, no conflicts) and re-verified that the arithmetic rewrite produces byte-identical serialize() output vs. the original int(''.join(str(d) for d in digits)) implementation.

Checked directly against the live DecimalType.serialize/deserialize round trip and against a side-by-side old-vs-new comparison harness covering:

  • zero in all its as_tuple() forms (0, -0, 0.0, 0E+5, 0E-5, ...)
  • negative values
  • negative scale / positive exponent (e.g. 1E+2, 9E+50)
  • very large magnitude (100-300 digit unscaled values)
  • very small magnitude / very negative exponent (e.g. 1E-200)
  • all-9s / boundary digit patterns
  • 20,000 randomized fuzz cases (random digit strings, sign, exponent in [-80, 80])

Result: all of the above match byte-for-byte between old and new implementations. Added an explicit Decimal('0') fixture to tests/unit/test_marshalling.py's marshalled_value_pairs (the existing table had no exact-zero case) so this is now covered by a permanent regression test rather than just ad-hoc verification.

One genuine (non-blocking) behavioral difference found: for Decimal('NaN') / Decimal('sNaN') (as_tuple() returns empty digits), the old code raised ValueError (from int('')), while the new code raises TypeError (from int32_pack(-exponent), since exponent is the string 'n' for NaN). This isn't a new correctness bug: CQL's decimal type has no representation for NaN, so both versions were already incapable of serializing it, and the old code was already inconsistent here too (it raised TypeError for Decimal('Infinity'), same as the new code does). No existing code or test depends on the specific exception type. Flagging for visibility; not treating it as blocking.

CI / review status

All CI checks pass (unit tests across all supported Python versions/event loops, wheel builds, build, snyk). No reviews or review threads on the PR to resolve.

Test results

  • tests/unit/test_types.py: pass
  • tests/unit/test_marshalling.py (incl. new zero-decimal case): pass
  • Full tests/unit/: 720 passed, 88 skipped, 0 failed

Amended the fix into the existing commit and force-pushed (kept as draft).

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Optimizes decimal serialization by replacing string conversion with arithmetic accumulation.

Changes:

  • Adds multiply-and-add digit conversion.
  • Tests zero serialization.
  • Adds performance and round-trip benchmarks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
cassandra/cqltypes.py Optimizes decimal digit conversion.
tests/unit/test_marshalling.py Adds zero-value coverage.
benchmarks/decimal_serialize.py Adds serialization benchmarks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread benchmarks/decimal_serialize.py Outdated
Comment on lines +81 to +82
("medium (10 digits)", Decimal("1234567890.0123456789")),
("large (30 digits)", Decimal("1" * 30 + ".0")),
…ialize

Replace int(''.join([str(d) for d in digits])) with a multiply-and-add
loop to convert Decimal digit tuples to integers. This avoids creating
per-digit string objects and the intermediate joined string.

Benchmark results (isolated digit conversion):
  - 3 digits:  2.66x faster
  - 20 digits: 1.55x faster
  - 31 digits: 1.34x faster

Signed-off-by: Yaniv Michael Kaul <yaniv.kaul@scylladb.com>
@mykaul
mykaul force-pushed the perf/decimal-serialize-arithmetic branch from a08da22 to babb234 Compare September 11, 2026 10:50
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.

2 participants