Skip to content

perf: Add microbenchmark results (run on m7i.2xlarge) - #5380

Draft
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:micro
Draft

perf: Add microbenchmark results (run on m7i.2xlarge)#5380
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:micro

Conversation

@andygrove

@andygrove andygrove commented Aug 16, 2026

Copy link
Copy Markdown
Member

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?

@andygrove andygrove changed the title Add microbenchmark results (run on m7i.2xlarge) perf: Add microbenchmark results (run on m7i.2xlarge) Aug 16, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 @@
================================================================================================

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

#5374 is now ready for review


## Environment

| Field | Value |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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 |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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 |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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.

@andygrove
andygrove marked this pull request as draft August 16, 2026 14: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