Skip to content

Commit

Permalink
Only update response headers if we have a new one (#37590)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jasontedor authored Jan 18, 2019
1 parent 3a96608 commit ed297b7
Showing 1 changed file with 11 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,17 @@ public void addResponseHeader(final String key, final String value) {
* @param uniqueValue the function that produces de-duplication values
*/
public void addResponseHeader(final String key, final String value, final Function<String, String> uniqueValue) {
threadLocal.set(threadLocal.get().putResponse(key, value, uniqueValue, maxWarningHeaderCount, maxWarningHeaderSize));
/*
* Updating the thread local is expensive due to a shared reference that we synchronize on, so we should only do it if the thread
* context struct changed. It will not change if we de-duplicate this value to an existing one, or if we don't add a new one because
* we have reached capacity.
*/
final ThreadContextStruct current = threadLocal.get();
final ThreadContextStruct maybeNext =
current.putResponse(key, value, uniqueValue, maxWarningHeaderCount, maxWarningHeaderSize);
if (current != maybeNext) {
threadLocal.set(maybeNext);
}
}

/**
Expand Down

0 comments on commit ed297b7

Please sign in to comment.