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

Avoid using JVM HashCode on Http 2 DefaultStream #13760

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

franz1981
Copy link
Contributor

Motivation:

relying on DefaultStream's hashCode forces calling into JVM's strategy for identity hash code

Modifications:

Use the stream's id to identify DefaultStream

Result:

Faster active stream insertion, saving producing JVM's hashcode

@franz1981
Copy link
Contributor Author

franz1981 commented Jan 3, 2024

This has been detected on the http 2 hello world-like benchmarking, see (in violet)

image

In case the stream's id is not good enough, given that DefaultHttp2Connection::createStream is single-threaded, I can use an instance int/long counter per each produced DefaultStream instance and use that one, instead.

@chrisvest
Copy link
Contributor

The stream id is a peer controlled value we read off the network. I'm hesitant to use it here, in case it opens us op to session hijacking vulnerabilities. I'd rather we pulled a hashcode off an atomic counter, and retained object identity for equality.

@franz1981
Copy link
Contributor Author

franz1981 commented Jan 3, 2024

@chrisvest looking at the code paths where it is used, it shouldn't happen (because we check upfront the expected next id to create, and removal happen from within the stream itself - which means that the creation id check had worked out), said that, better be safe then sorry, and we can skip using an atomic counter here, given that the default stream code path is single threaded (you can verify it by checking the stream id check before, which uses thread unsafe fields)

@franz1981
Copy link
Contributor Author

franz1981 commented Jan 3, 2024

Just fyi, @chrisvest I'm recently receiving hints from users which wrongly compare http 1.1 Vs 2 (very wrongly, cause the power of http 2 is beyond hello world use cases), but effectively there are few low hanging fruits (and some not so low) which could reduce a lot the stream management cost (creation and notification), plus other improvements related known header name lookups (addresses elsewhere, in another pr), and I am trusty, we can get very similar performance by fixing everything, assuming code won't suffer too much from this effort.

@franz1981
Copy link
Contributor Author

@chrisvest I've added a commit which is leaving equals as it is and allowing ConnectionStream (which extends DefaultStream, but is not placed in the same linked hash set) to behave as it is now.

Motivation:

relying on DefaultStream's hashCode forces calling into JVM's strategy for identity hash code

Modifications:

Use the stream's id to identify DefaultStream

Result:

Faster active stream insertion, saving producing JVM's hashcode
@normanmaurer normanmaurer added this to the 4.1.105.Final milestone Jan 12, 2024
@normanmaurer normanmaurer merged commit 4dea3f7 into netty:4.1 Jan 12, 2024
15 checks passed
@normanmaurer
Copy link
Member

@franz1981 thanks a lot!

normanmaurer pushed a commit that referenced this pull request Jan 12, 2024
Motivation:

relying on DefaultStream's hashCode forces calling into JVM's strategy
for identity hash code

Modifications:

Use the stream's id to identify DefaultStream

Result:

Faster active stream insertion, saving producing JVM's hashcode
franz1981 added a commit to franz1981/netty that referenced this pull request Feb 9, 2024
Motivation:

relying on DefaultStream's hashCode forces calling into JVM's strategy
for identity hash code

Modifications:

Use the stream's id to identify DefaultStream

Result:

Faster active stream insertion, saving producing JVM's hashcode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants