Repository navigation
Improve bulker performance - #7190
Conversation
…date BenchmarkFlushSearch, BenchmarkFlushRead, and BenchmarkFlushAPIKeyUpdate directly exercise the three flush paths that had no allocation baseline. Each benchmark uses a fixedTransport (zero per-request parsing overhead) and a pre-built queue so measurements isolate the flush functions themselves. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace *apm.SpanLink pointer with value type + hasSpanLink bool on bulkT and optionsT, eliminating one heap allocation per bulk op. Remove withAPMLinkedContext closure pattern; extract APM transaction context inline at each call site instead. Fix multiWaitBulkOp to actually set span links on queued items (previously called withAPMLinkedContext but never propagated the result) and add a caller-level APM span for multi-ops. Pool flush buffers via flushBufPool sync.Pool across flushBulk, flushSearch, and flushRead to avoid per-flush buffer allocation. Pre-allocate span link slices with queue.cnt capacity. Pre-compute APM span name strings as package-level arrays to eliminate fmt.Sprintf on every flush. Size-hint all six maps in flushUpdateAPIKey with queue.cnt. Replace double-decode of NDJSON meta in flushUpdateAPIKey with bytes.Cut to skip the meta line, then unmarshal only the body. Remove the redundant outer APM span from Read (ReadRaw already spans). Benchstat results (n=10, benchtime=3s): - FlushRead: -22% time, -64% bytes - FlushAPIKeyUpdate: -49% time, -62% bytes - FlushSearch: -14% time, -52% bytes - MockBulk/1: -5% time, -19% bytes, -13% allocs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
|
I re-ran bechmarks with
Overall the removal of withAPMLinkedContext and less JSON decoders should result in less GC intervention. |
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
a1e4af6 to
738a2f4
Compare
This comment has been minimized.
This comment has been minimized.
TL;DR
Remediation
Investigation detailsRoot CauseClassification: Inconclusive / missing failure data. The local artifact at The PR file list is limited to Evidence
Verification
Follow-upIf the full log shows a specific failing suite, rerun this detective on that output; the current tail is insufficient to identify a defensible code or test fix. What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
blakerouse
left a comment
There was a problem hiding this comment.
These are big improvements, good work on this.
What is the problem this PR solves?
Improve bulker performance by lowering the number of allocations.
How does this PR solve the problem?
APM Improvements:
APM span handling has changed from using a pointer to a directly allocated span per bulkT (removing 1 heap allocation)
Replaced
withAPMLinkedContextwith direct inline extraction (avoids allocating a slice and closure)Pre-allocate span links
Pre-compute span name arrays
Removed span in Read operation -> read always calls ReadRaw
Bug fix
Correctly trace calls in
opMulti.goOther improvements
OpApiKey.go - use queue.cnt as a size hint to pre-allocate space
flushUpdateAPIKey - replace json.Decoder call that discards decoded bytes with a
bytes.Cutin order to discard metadata.How to test this PR locally
Additional benchmarks have been added with dae7d64.
A benchstat comparison between dae7d64 and 11ff6f2 produced the following results:
There are major improvements across the bulker except for the Multi operations (due to the bug fix)
Design Checklist
I have included fail safe mechanisms to limit the load on fleet-server: rate limiting, circuit breakers, caching, load shedding, etc.Checklist
I have commented my code, particularly in hard-to-understand areasI have made corresponding changes to the documentationI have made corresponding change to the default configuration files./changelog/fragmentsusing the changelog tool