Skip to content

test: broaden blob/docstore/pubsub benchmark coverage - #592

Open
iamabhilaksh wants to merge 3 commits into
salesforce:mainfrom
iamabhilaksh:test/benchmark-coverage-existing-suites
Open

test: broaden blob/docstore/pubsub benchmark coverage#592
iamabhilaksh wants to merge 3 commits into
salesforce:mainfrom
iamabhilaksh:test/benchmark-coverage-existing-suites

Conversation

@iamabhilaksh

Copy link
Copy Markdown
Contributor

Summary

Broadens the already-merged JMH suites with mutate/delete coverage that was missing. Test-file-only — no pom change, since these suites already carry JMH.

  • blob (sync) AbstractBlobBenchmarkTest: benchmarkBulkDelete
  • blob (async) AbstractAsyncBlobBenchmarkTest: benchmarkDeleteDirectory, benchmarkBulkDelete
  • docstore AbstractDocstoreBenchmarkTest: benchmarkCreate, benchmarkReplace
  • pubsub AbstractPubsubBenchmarkTest: benchmarkGetAttributes

Measurement caveat

The blob benchmarkBulkDelete and benchmarkDeleteDirectory bundle the upload staging into the measured method — the shared @State precludes per-invocation staging of a delete-only target, so the number is "stage + delete", not delete alone. Documented in-source. benchmarkCreate tracks its keys and deletes them in teardown (no doc accumulation); benchmarkReplace overwrites pre-seeded keys.

Testing proof

These extend suites that already run in the existing benchmark harness. Test-compile + checkstyle clean on blob/docstore/pubsub -client modules; the new methods follow the same @State/@threads patterns as the surrounding benchmarks.

JMH config note

Class-level annotations are the local-run baseline; the chameleon pipeline overrides via its own BenchmarkRunner.

Downstream

These modules are already vendored in sfdc-bazel, but the vendored copies are frozen at sync time — picking up these new methods needs a re-sync PR; not automatic.

Merge order

Independent of the other PRs.

GUS: W-23830177

Extends the already-merged JMH suites with mutate/delete coverage:
- blob (sync): benchmarkBulkDelete
- blob (async): benchmarkDeleteDirectory, benchmarkBulkDelete
- docstore: benchmarkCreate, benchmarkReplace
- pubsub: benchmarkGetAttributes

Test-file-only; these suites already carry JMH so no pom change. The blob
bulk-delete/delete-directory benchmarks bundle upload staging into the measured
method (shared @State precludes delete-only staging), documented in-source.

Story: https://gus.lightning.force.com/lightning/r/ADM_Work__c/a07EE00002hPHP4YAO/view (W-23830177)
@codecov-commenter

codecov-commenter commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.58%. Comparing base (aea85ed) to head (b71d4ce).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #592      +/-   ##
============================================
- Coverage     83.61%   83.58%   -0.03%     
  Complexity      674      674              
============================================
  Files           215      215              
  Lines         15010    14930      -80     
  Branches       2076     2076              
============================================
- Hits          12550    12479      -71     
+ Misses         1636     1628       -8     
+ Partials        824      823       -1     
Flag Coverage Δ
unittests 83.58% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

protected static final int MESSAGE_AVAILABILITY_DELAY_MS = 1000;

// @Param for batch ack benchmarks — drives both benchmarkBatchAck and benchmarkLargeBatchAck
@Param({"1", "10"})

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🚨 This file is the pre-#586 version plus one method — the pubsub half of this PR reverts its own parent commit

On this branch, git diff 798bf1d9 HEAD -- pubsub/pubsub-client/.../AbstractPubsubBenchmarkTest.java reports +17/−0, and those 17 lines are entirely benchmarkGetAttributes. 798bf1d9 is the commit before #586, and #586 (aea85edd) is this branch's direct parent, merged about 78 minutes earlier. So every change #586 made to this file is undone here. That reads like a conflict resolved the wrong way rather than a decision — the commit message describes only the new coverage and doesn't mention it.

Three things come back:

The class-level @Param on this line. The class is itself @State(Scope.Benchmark), so JMH cross-products batchSize over every benchmark in it, not just the two that read it. The sibling suite in this repo documents exactly this trap, and this PR doesn't touch it: "Method-level injection (vs. a class-level @PARAM field) prevents JMH from cross-producting batchSize across the 12 non-batch benchmarks and doubling suite runtime for no signal" (docstore-client/.../AbstractDocstoreBenchmarkTest.java:75-83). Concretely it takes pubsub from 24 to 52 runs per provider — roughly +8 minutes of measurement each across the three pubsub subclasses, and 84 extra forks that each re-run a @Setup(Level.Trial) seeding 50 messages at the 3–5 s per publish this file documents at :65. On those numbers the setup cost exceeds the measurement cost by about an order of magnitude.

benchmarkLargeBatchAck, now a provable duplicate. #586 removed getMaxBatchAckSize() and GcpPubsubBenchmarkTest's return 1000 override together. This PR restores only the base method, and git grep getMaxBatchAckSize at this head finds no override anywhere — so it returns BATCH_SIZE_SMALL (10) for every provider, Math.min(batchSize, 10) == batchSize at both param values, and :421-437 is statement-for-statement identical to benchmarkBatchAck. That's 4 extra forks per provider producing a duplicate series.

The javadoc at :111 and :413 — "AWS: 10 messages per batch, GCP: 1000" — which #586 removed as provider-specific documentation in a cloud-agnostic module, and which is now also factually wrong about the code it annotates, since nothing can reach 1000.

Keeping benchmarkGetAttributes and dropping the rest of the pubsub diff resolves all three. Worth noting CI can't catch this: per #587 the JMH annotation processor doesn't currently run at all, so a full revert of #586 sails through green.

The other three files in this PR look fine to me — the new benchmarks participate correctly in the existing teardown tracking, @Fork(1) plus per-trial re-seeding means none of them can corrupt the pre-seeded fixtures the existing benchmarks read, and every added benchmark works on all the existing provider subclasses.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, and thanks for the depth here — you're exactly right. This was a bad conflict resolution against #586, not a deliberate change: the pubsub file got rebuilt on the pre-#586 version with only benchmarkGetAttributes layered on, which silently reverted everything #586 removed.

Fixed in 8df2abbf by restoring #586's version of the file and re-applying only the new method. That drops all three at once — the class-level batchSize @Param (back to your scoped BatchParams nested @State), benchmarkLargeBatchAck + getMaxBatchAckSize (which, with no GCP override left, had collapsed into a duplicate of benchmarkBatchAck), and the provider-specific javadoc. The pubsub diff is now 17+/0−, just benchmarkGetAttributes.

And agreed on the CI blind spot — since the JMH annotation processor isn't running yet (#587), I verified locally with a clean test-compile across pubsub-client + the AWS/GCP subclasses instead of relying on green CI.

…hmark

The pubsub half of this PR re-introduced everything salesforce#586 removed from
AbstractPubsubBenchmarkTest (class-level batchSize @PARAM, getMaxBatchAckSize,
benchmarkLargeBatchAck, provider-specific javadoc) via a bad conflict
resolution. Restore salesforce#586's version and keep only the new benchmarkGetAttributes
method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants