perf: Add microbenchmark results (run on m7i.2xlarge) - #5380
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
Reviewed the complete 37-file benchmark-results snapshot at commit f94a1ae3a9ecb03ad0cffdab86b64f51a8b43b11, including the recorded run metadata, the benchmark implementations, the separate runner/tooling PR, current CI logs, and source-release validation. The proposed baseline is useful in principle, but the current snapshot has merge/release-blocking generated-file handling and several reproducibility, completeness, and measurement-validity problems.
Prior state and problem
The repository has Spark/Comet microbenchmark suites but no checked-in, machine-identified snapshot of their results. Without a reproducible baseline, subsequent expression, reader, shuffle, and operator changes cannot be compared consistently for performance regressions.
Design approach
This change publishes 35 generated text artifacts under benchmarks/results/micro/, alongside a README and a generated run manifest recording the m7i.2xlarge environment, measured commit, per-suite duration, and completion status. The design treats all suite outputs as one machine-specific snapshot that should be replaced together.
Correctness / compatibility analysis
The result artifacts do not change runtime execution, but their integration does affect required Apache RAT, Markdown formatting, and source-release checks. The current exact-head Preflight run already rejects all 35 generated text files; the release tooling has a second independent RAT exclusion list, and the generated Markdown manifest also lacks the formatter exemption present in the separate runner PR. Several artifacts originate from failed or timed-out suites, while specific reported conversion and Iceberg serialization measurements do not exercise the operations their labels claim.
Key design decisions
The snapshot records a specific EC2 instance type, JVM, operating system, heap size, measured commit, and suite status, which are the right metadata for comparisons. However, the runner, EC2 instructions, configurable heap, and generated-file exemptions currently live in the still-open prerequisite PR rather than in this change, so the documented workflow is unavailable at the reviewed head.
Implementation sketch
Each Scala benchmark writes a generated *-results.txt file containing Spark and Comet timing/throughput rows, and the proposed collection workflow copies those files into the tracked results directory and emits RUN-INFO.md. CI first executes the root Maven RAT check, then checks Markdown formatting; source releases independently run the release RAT filter over a git archive tarball.
Behavioral changes worth calling out
The change makes generated benchmark output part of normal CI and source-release validation for the first time. It also establishes apparent baselines for failed cast/reader suites and a timed-out shuffle suite, including missing coverage that readers could otherwise mistake for a completed measurement set.
Suggested improvements
Merge or include the runner prerequisites before publishing the snapshot; handle generated results in both RAT validation paths and the generated Markdown formatting check; rerun or omit failed suites; give the shuffle workload enough time or shard it; and correct the always-null date-conversion and placeholder-only Iceberg serialization comparisons before treating these measurements as regression baselines.
| @@ -0,0 +1,390 @@ | |||
| ================================================================================================ | |||
There was a problem hiding this comment.
[P1] Handle generated results in both Apache RAT validation paths. All 35 new *-results.txt files lack recognized ASF headers, and the existing Maven exclusion **/benchmarks/*.txt does not match benchmarks/results/micro/. The current exact-head Preflight job already fails with Unapproved: 35 / Too many files with unapproved license: 35, which skips the downstream build, benchmark, and Spark/Iceberg test jobs. Source releases also run a separate RAT filter using dev/release/rat_exclude_files.txt, so changing only pom.xml would still leave release creation broken. Please add narrowly scoped generated-result exclusions to both validation paths, or emit recognized ASF headers for every result file.
| produced the results files next to it. Results are only comparable across runs on the same | ||
| instance type, so replace the whole directory in one commit rather than updating individual files. | ||
|
|
||
| Results are produced and collected with `benchmarks/micro/run.py`. See |
There was a problem hiding this comment.
[P2] Merge the benchmark runner and setup before publishing its outputs. Neither the documented benchmarks/micro/run.py nor the linked docs/source/contributor-guide/benchmarking_micro_ec2.md exists at this PR head; both paths return 404. The measured commit recorded in RUN-INFO.md belongs to the separate, still-open runner PR #5374, which also contains the configurable BENCH_HEAP support needed to reproduce the documented 8g heap; this PR's Makefile still hardcodes -Xmx20g. As submitted, contributors cannot run the documented collection workflow or reproduce the recorded configuration from this tree. Please merge/include the prerequisite tooling first, or provide working in-tree instructions and equivalent configuration.
|
|
||
| ## Environment | ||
|
|
||
| | Field | Value | |
There was a problem hiding this comment.
[P2] Add the missing formatter exemption for the generated run manifest. RUN-INFO.md contains unaligned generated Markdown tables, but this PR does not add it to .prettierignore; the exact-head Preflight workflow runs prettier --check "**/*.md" immediately after the RAT step. The prerequisite runner branch already contains a dedicated commit adding benchmarks/results/micro/RUN-INFO.md to .prettierignore because this manifest is generated on a machine without Prettier, but that exemption was omitted here. Consequently fixing the current RAT failure would expose a second Markdown-formatting failure. Please include the prerequisite ignore entry or format and preserve the generated manifest accordingly.
| | CometCastBooleanBenchmark | ok | 4m17s | CometCastBooleanBenchmark-results.txt | | ||
| | CometCastNumericToNumericBenchmark | ok | 12m24s | CometCastNumericToNumericBenchmark-results.txt | | ||
| | CometCastNumericToStringBenchmark | ok | 2m26s | CometCastNumericToStringBenchmark-results.txt | | ||
| | CometCastNumericToTemporalBenchmark | failed | 7s | CometCastNumericToTemporalBenchmark-results.txt | |
There was a problem hiding this comment.
[P2] Do not publish failed suites as completed regression baselines. Three listed suites failed but their partial output was nevertheless committed: CometCastNumericToTemporalBenchmark-results.txt contains only a four-line heading and zero measurements; CometCastTemporalToNumericBenchmark-results.txt contains only 4 of its 16 defined cases; and CometReadBenchmark-results.txt stops at the encrypted-scan heading, omitting the remaining encrypted, decimal, dictionary, filter, wide-column, and string-scan groups. Keeping these files beside successful artifacts makes entire benchmark families unavailable for future comparisons while presenting them as published baselines. Please fix and rerun the failed suites, or exclude their artifacts until valid results can be collected.
| | CometReadBenchmark | failed | 3m54s | CometReadBenchmark-results.txt | | ||
| | CometRegExpBenchmark | ok | 1m29s | CometRegExpBenchmark-results.txt | | ||
| | CometRegExpExtractBenchmark | ok | 3m37s | CometRegExpExtractBenchmark-results.txt | | ||
| | CometShuffleBenchmark | timeout | 1h00m00s | CometShuffleBenchmark-results.txt | |
There was a problem hiding this comment.
[P2] Complete or shard the shuffle benchmark before publishing its baseline. The shuffle suite hit its one-hour timeout while still processing struct cases. Its committed results therefore omit all subsequently scheduled dictionary shuffle, ordinary and randomized shuffle, wide shuffle, and wide range-partition shuffle groups, including the native-shuffle comparisons those groups contain. This is a substantial coverage gap in precisely the workload family this baseline should protect, not merely a slower final sample. Please increase the suite timeout or split/shard the workload, then collect a completed shuffle artifact.
|
|
||
| OpenJDK 64-Bit Server VM 17.0.20+8-LTS on Linux 6.18.39-79.141.amzn2023.x86_64 | ||
| Intel(R) Xeon(R) Platinum 8488C | ||
| CAST Date to BYTE: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative |
There was a problem hiding this comment.
[P2] Remove always-null DATE casts from the conversion baseline. The benchmark disables ANSI mode, under which Spark casts from DATE to BYTE, SHORT, INT, and LONG always return NULL; Comet explicitly special-cases the same always-null conversions. Consequently all four reported 1.2x-1.3x results in this file measure scanning and null materialization rather than any date-to-number conversion, so they cannot detect regressions in real temporal conversion. Please replace them with a meaningful operation such as unix_date(c_date), or omit these misleading cases from the published baseline.
|
|
||
| OpenJDK 64-Bit Server VM 17.0.20+8-LTS on Linux 6.18.39-79.141.amzn2023.x86_64 | ||
| Intel(R) Xeon(R) Platinum 8488C | ||
| IcebergScan serde (30000 partitions, 30000 tasks): Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative |
There was a problem hiding this comment.
[P2] Do not label placeholder serialization as 30,000-task Iceberg serde. The benchmark obtains 30,000 FileScanTasks but measures CometIcebergNativeScan.convert(...) and serialization of its returned operator. Both the exact PR-head implementation and the measured source commit explicitly document that convert creates only a minimal metadata placeholder with an empty partition field; actual task and partition serialization occurs later in serializePartitions(), which these cases never invoke. The reported 102-691 ns/op figures therefore serialize zero of the claimed 30,000 tasks and materially misrepresent the real task-scaling cost. Please benchmark serializePartitions()/the actual task payload, or relabel these rows as constant-size placeholder conversion.
Which issue does this PR close?
Part of #5373
Benchmarks were run using the tooling in #5374, which we should merge first.
Rationale for this change
Add baseline microbenchmark results - run on m7i.2xlarge
Note that many of the benchmarks are flawed - they all test parquet scan and c2r heavily, not just the expressions they intend to measure. I am working on fixing that in other PRs, but we need a baseline to start with.
What changes are included in this PR?
How are these changes tested?