[Composite Terms Aggregation] Optimize by removing unnecessary object allocations #18531
+43
−21
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
|
{"run-benchmark-test": "id_4"} |
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/3470/ . Final results will be published once the job is completed. |
|
❌ Gradle check result for cfdfa76: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/3470/
|
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/122/
|
sandeshkr419
requested review from
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta and
sachinpkale
as code owners
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/138/
|
|
The PR looks good. Some points: Slot object reuse - is this per-thread or shared? If shared, need synchronization. If per-thread, document the threading model assumptions clearly. I dont see tests for the core optimizations. I would suggest explicit test that validates:
Also, this changes object lifecycle in hot path. Validate:
Where's the actual slot reuse implementation? The PR description mentions the optimization but need to see the specific code changes to evaluate correctness. Regarding benchmarks, standard microbenchmarks may not capture GC improvements. Consider longer-running tests with memory pressure to validate allocation reduction benefits. |
atris
reviewed
Aug 1, 2025
|
{"run-benchmark-test": "id_4"} |
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/3985/ . Final results will be published once the job is completed. |
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/3985/
|
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/141/
|
|
Latest results 90th percentile service time composite-terms 223.699 217.607 -6.09272 ms Btw, I think you had mem allocation profile results using async, you can paste it? Otherwise looks good to me. |
|
@asimmahmood1 The major changes are observed in improved throughput and less memory usage. The PR benchmark runs are capped at 2 ops/sec, because of which the difference wasn't noted there much. In all my multiple local runs, I could see a better throughput (0.88 to 0.98 ops/s) which is nice. Since the code changes don't involve major logical changes, the diff is service time improvement is minor. Also, memory profiling on initial analyzed chunks is also improved. Before: 
After: 
|
Signed-off-by: Sandesh Kumar <[email protected]>
|
@atris Thanks for taking a look. The objects inside an Aggregator class are not shared between threads. Also, I don't suspect memory leaks because of this change. |
This was referenced Aug 5, 2025
asimmahmood1
approved these changes
Aug 5, 2025
expani
approved these changes
Aug 5, 2025
prudhvigodithi
approved these changes
Aug 5, 2025
github-project-automation
bot
moved this from Todo
to In Progress
in Performance Roadmap
github-project-automation
bot
moved this from In Progress
to Done
in Performance Roadmap
23 tasks
65 tasks
vinaykpud
pushed a commit
to vinaykpud/OpenSearch
that referenced
this pull request
Sep 26, 2025
… allocations (opensearch-project#18531) * reuse slot objects for lookups Signed-off-by: Sandesh Kumar <[email protected]> * refactor streams to for loop Signed-off-by: Sandesh Kumar <[email protected]> --------- Signed-off-by: Sandesh Kumar <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Description
Trying to achieve micro-improvements with some objects overheads in a composite terms aggregation request:
sourceConfigswith the help ofstreams5 times. Changing this to a singlefor loopto avoid multiple traversals. Also, the size of thesourceConfigsis the size of fields in composite aggregation request, so most use cases will see 2-3 fields, for which the overhead of 5 streams doesn't make sense.Regarding concurrent search, when intra segment concurrent search will be introduced, all aggregations will break naturally, so that will be another problem on its own which will be a follow-up on. #18879
Related Issues
Resolves #18440
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.