Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

codec-http2: Lazily translate cookies for HTTP/1 #9251

Merged
merged 4 commits into from Jun 19, 2019

Conversation

Projects
None yet
4 participants
@kevinoliver
Copy link
Contributor

commented Jun 17, 2019

Motivation:

For HTTP/2 messages with multiple cookies HttpConversionUtil.addHttp2ToHttpHeaders spends a good portion of time creating throwaway StringBuilders.

Modification:

Handle cookies lazily by building up a single StringBuilder and then converting it to the H1 header at the end.

Result:

Less allocations.

@netty-bot

This comment has been minimized.

Copy link

commented Jun 17, 2019

Can one of the admins verify this patch?

@vkostyukov
Copy link
Contributor

left a comment

LGTM

@kevinoliver

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

allocation-flamegraph

Attached is a screenshot from an allocation flamegraph where this was observed.

output.set(COOKIE,
(existingCookie != null) ? (existingCookie + "; " + value) : value);
if (cookies == null) {
cookies = new StringBuilder(value.length());

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 18, 2019

Member

I wonder if we should allocate a bit more then value.length here to prevent costly "re-allocations" when we have multiple values.

This comment has been minimized.

Copy link
@kevinoliver

kevinoliver Jun 18, 2019

Author Contributor

it's tricky to guess what the right size should be. i went with adding 512 to value.length but this is arbitrary.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 18, 2019

Member

Btw did you consider using InternalThreadLocalMap.stringBuilder() ?

This comment has been minimized.

Copy link
@kevinoliver

kevinoliver Jun 18, 2019

Author Contributor

nope. first i've heard of it. looks like it'd be perfect here. will update.

This comment has been minimized.

Copy link
@vkostyukov

vkostyukov Jun 18, 2019

Contributor

TIL. This is great.

} else {
output.add(name, value);
}
}
}

public void translateCookies() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 18, 2019

Member

I wonder if it would be better to have this code encapsulated in one method and so make it less error-prone to use.

Basically change the translate(Entry<....>) method to translate(Iterable<...>) and then do the for-loop in there + the code that you added in translateCookies at the end. This way it will be impossible to forget adding the cookies at the end....

@kevinoliver WDYT ?�

This comment has been minimized.

Copy link
@kevinoliver

kevinoliver Jun 18, 2019

Author Contributor

yeah i went with least code churn but i prefer this.

going to have a new "public" method translateHeaders(...) that does all this.

@kevinoliver

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@normanmaurer updated PR with your feedback, thanks.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@netty-bot test this please

@kevinoliver

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Looks to me like that centos6-java12 failure is unrelated.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@netty-bot test this please

@normanmaurer normanmaurer added this to the 4.1.37.Final milestone Jun 18, 2019

@kevinoliver

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

updated to push the stringbuilder into a local variable.

fwiw, that centos6-java8 CI failure looks unrelated to me.

@normanmaurer normanmaurer self-requested a review Jun 18, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@netty-bot test this please

@normanmaurer normanmaurer merged commit c32c9b4 into netty:4.1 Jun 19, 2019

3 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

@kevinoliver thanks a lot!

normanmaurer added a commit that referenced this pull request Jun 19, 2019

codec-http2: Lazily translate cookies for HTTP/1 (#9251)
Motivation:

For HTTP/2 messages with multiple cookies HttpConversionUtil.addHttp2ToHttpHeaders spends a good portion of time creating throwaway StringBuilders.

Modification:

Handle cookies lazily by using a ThreadLocal StringBuilder and then converting it to the H1 header at the end.

Result:

Less allocations.
@kevinoliver

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@kevinoliver thanks a lot!

you're welcome. thanks for the code reviews.

@kevinoliver kevinoliver deleted the kevinoliver:optimize-h2-to-h1-translator branch Jun 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.