deserializeSafeFromHeader uses context.getHeader(headerName) instead of context.getHeaders()#2768
Conversation
parasjain1
requested review from
DarshitChanpura,
RyanL1997,
cliu123,
cwperks,
davidlago,
peternied,
reta and
stephen-crawford
as code owners
krishna-ggk
left a comment
krishna-ggk
left a comment
There was a problem hiding this comment.
Changes are good - can you share the experiment results?
|
|
||
| String headerValue = null; | ||
|
|
||
| Map<String, String> headers = context.getHeaders(); | ||
| if (!headers.containsKey(headerName) || (headerValue = headers.get(headerName)) == null) { | ||
| return null; | ||
| } | ||
|
|
||
| if (isInterClusterRequest(context) || isTrustedClusterRequest(context) || isDirectRequest(context)) { | ||
| return headerValue; | ||
| return context.getHeader(headerName); | ||
| } | ||
|
|
There was a problem hiding this comment.
It is surprising that this small change makes a sizeable difference.
There was a problem hiding this comment.
Changes are good - can you share the experiment results?
- HeaderHelper.deserializeSafeFromHeader ‘s overall CPU usage dropped to 0.52% from 5%-12.5% seen across multiple CPU profiles obtained for prior runs.
- Search Latency saw a drop of ~75%, Search Throughput increased by ~175%
- Indexing Latency dropped by 70%, throughput increased by more than ~50%.
This was tested on a cluster running 3 m5.2xlarge data nodes with OS1.3
Results can vary based on the type of workload being executed.
Codecov Report
@@ Coverage Diff @@
## main #2768 +/- ##
=======================================
Coverage ? 61.33%
Complexity ? 3409
=======================================
Files ? 272
Lines ? 18846
Branches ? 3295
=======================================
Hits ? 11560
Misses ? 5686
Partials ? 1600
|
|
@parasjain1 Can you sign the commits? |
|
@parasjain1 This change looks good to me. A lot less work is being done now by only reading a single header instead of reading all of the ThreadContext headers into memory on every request to |
|
Looking into the issue with plugin install. I suspect it could be related to this PR: opensearch-project/OpenSearch#7465 Edit: The minified distribution is outdated and does not include the change above yet. I will re-run the checks when updated artifacts are available. |
…ead of `context.getHeaders()` Signed-off-by: Paras Jain <parasjaz@amazon.com>
Signed-off-by: Paras Jain <parasjaz@amazon.com>
Done. |
DarshitChanpura
left a comment
DarshitChanpura
left a comment
There was a problem hiding this comment.
Great fix @parasjain1 !!
…ead of `context.getHeaders()` (opensearch-project#2768) (opensearch-project#2771) (cherry picked from commit 7c4e06d) Co-authored-by: Paras Jain <parasjain.jain@outlook.com>
…ead of `context.getHeaders()` (opensearch-project#2768)
…ead of `context.getHeaders()` (opensearch-project#2768) Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>
…ead of `context.getHeaders()` (opensearch-project#2768) Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>
…ead of `context.getHeaders()` (opensearch-project#2768)
…ead of `context.getHeaders()` (opensearch-project#2768) Signed-off-by: Sam <samuel.costa@eliatra.com>
5 participants
Description
Category: Performance Bug Fix
Why these changes are required?
HeaderHelper.deserializeSafeFromHeaderis invoked O(segment) times during every search request and introduced a huge overhead. This optimisation has resulted in good gains with respect to search throughput and latency in our experiments.Issues Resolved
HeaderHelper.getSafeFromHeaderuses ThreadContext.getHeaders() causing major performance overhead #2757Issues Resolved
Testing
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.