perf: avoid repeated decimal promotion in expression serialization - #5736
Open
peterxcli wants to merge 1 commit into
Open
perf: avoid repeated decimal promotion in expression serialization#5736peterxcli wants to merge 1 commit into
peterxcli wants to merge 1 commit into
Conversation
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.
Which issue does this PR close?
Closes #5248.
Rationale for this change
exprToProtopromotes the complete decimal expression tree before serialization. Array and bitwise serdes re-entered that public method for children of the promoted tree, repeating traversal and allocation. Promotion is already idempotent, but that does not eliminate the redundant work.What changes are included in this PR?
exprToProtoInternal.How are these changes tested?
make core../mvnw test-compile -DskipTests.SPARK_LOCAL_IP=127.0.0.1 ./mvnw test -Dtest=none -Dsuites=org.apache.spark.sql.comet.CometDecimalPromotionSuite,org.apache.comet.CometArrayExpressionSuite— 62 tests passed.Serialization microbenchmark
Measured before rebasing, against
75fdddc9285ec61c0cd326977c61dd41fca39a8b. The rebase onto7e1984399does not change the measured serializer paths. The baseline uses the originalarrays.scalaandbitwise.scalacompiled into a classpath overlay; the patched runs use this change, with the same dependencies and benchmark harness. Class origins were verified in each process.Spark 4.1.3, Scala 2.13.17, Zulu JDK 21.0.6, macOS aarch64; JVM flags
-Xms1g -Xmx1g -XX:ActiveProcessorCount=2. Three JVM runs per version in baseline/patched/patched/baseline/baseline/patched order. Each case warms up for two seconds, then measures seven batches of 1,000 serializations. Results are the median of the three per-process medians, per serialization. Allocations use the current thread'sThreadMXBeancounter; a volatile sink consumes the protobuf output.Every case contains an eight-add decimal chain. Array cases wrap it in
CreateArrayand the indicated number ofReversenodes, thenArrayContains. Bitwise cases cast it to integer and nestBitwiseNot. The control serializes only the decimal arithmetic. Inputs are reused; expression construction and query execution are outside the measured region.These are synthetic serializer measurements on a shared development machine, not end-to-end SQL speedups. The unchanged control's per-process time medians range from 8.71–10.53 µs before and 8.76–9.29 µs after, so small timing differences should be treated as noise. Small allocation differences in the control can reflect JVM optimization differences.