Only update response headers if we have a new one#37590
Merged
jasontedor merged 2 commits intoelastic:masterfrom Jan 18, 2019
Merged
Only update response headers if we have a new one#37590jasontedor merged 2 commits intoelastic:masterfrom
jasontedor merged 2 commits intoelastic:masterfrom
Conversation
Currently when adding a response header, we do some de-duplication, and maybe drop the header on the floor if we have reached capacity. Yet, we still update the thread local tracking the response headers. This is really expensive because under the hood there is a shared reference that we synchronize on. In the case of a request processed across many shards in a tight loop, this contention can be detrimental to performance. We can avoid updating the thread local in these cases though, when the response header is duplicate of one that we have already seen, or when it's dropped on the floor. This commit addresses these performance issues by avoiding the unnecessary set.
Collaborator
|
Pinging @elastic/es-core-infra |
Member
Author
|
I still need to do benchmarking on this, so consider this ready for review but WIP. I will do the benchmarking and update this thread with the results ASAP. |
…-response-header-performance * elastic/master: Remove Redundant RestoreRequest Class (elastic#37535) Create specific exception for when snapshots are in progress (elastic#37550) Mute UnicastZenPingTests#testSimplePings [DOCS] Adds size limitation to the get datafeeds APIs (elastic#37578) Fix assertion at end of forceRefreshes (elastic#37559) Propagate Errors in executors to uncaught exception handler (elastic#36137) [DOCS] Adds limitation to the get jobs API (elastic#37549) Add set_priority action to ILM (elastic#37397) Make recovery source send operations non-blocking (elastic#37503) Allow field types to optimize phrase prefix queries (elastic#37436) Added fatal_exception field for ccr stats in monitoring mapping. (elastic#37563) Fix testRelocateWhileContinuouslyIndexingAndWaitingForRefresh (elastic#37560) Moved ccr integration to the package with other ccr integration tests. Mute TransportClientNodesServiceTests#testListenerFailures Decreased time out in test Fix erroneous docstrings for abstract bulk by scroll request (elastic#37517) SQL: Rename SQL type DATE to DATETIME (elastic#37395) Remove the AbstracLifecycleComponent constructor with Settings (elastic#37523)
Member
Author
|
@elasticmachine run gradle build tests 1 |
Member
Author
|
@elasticmachine run the docbldesx |
Member
Author
|
@elasticmachine run gradle builds tests 1 |
Member
Author
|
@elasticmachine run the gradle build tests 1 |
This was referenced Jan 18, 2019
danielmitterdorfer
approved these changes
Jan 18, 2019
Member
danielmitterdorfer
left a comment
There was a problem hiding this comment.
Thanks for addressing this. LGTM
jasontedor
added a commit
that referenced
this pull request
Jan 18, 2019
Currently when adding a response header, we do some de-duplication, and maybe drop the header on the floor if we have reached capacity. Yet, we still update the thread local tracking the response headers. This is really expensive because under the hood there is a shared reference that we synchronize on. In the case of a request processed across many shards in a tight loop, this contention can be detrimental to performance. We can avoid updating the thread local in these cases though, when the response header is duplicate of one that we have already seen, or when it's dropped on the floor. This commit addresses these performance issues by avoiding the unnecessary set.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Currently when adding a response header, we do some de-duplication, and maybe drop the header on the floor if we have reached capacity. Yet, we still update the thread local tracking the response headers. This is really expensive because under the hood there is a shared reference that we synchronize on. In the case of a request processed across many shards in a tight loop, this contention can be detrimental to performance. We can avoid updating the thread local in these cases though, when the response header is duplicate of one that we have already seen, or when it's dropped on the floor. This commit addresses these performance issues by avoiding the unnecessary set.
Relates #35754
Relates #37530