Use fast HPACK comparisons when not checking sensitive headers #9259
Conversation
|
Can one of the admins verify this patch? |
| @@ -59,21 +60,16 @@ final int size() { | |||
|
|
|||
| @Override | |||
| public final int hashCode() { | |||
| // TODO(nmittler): Netty's build rules require this. Probably need a better implementation. | |||
| return super.hashCode(); | |||
| throw new UnsupportedOperationException(); | |||
There was a problem hiding this comment.
Isn’t it possible that the user calls this somehow ? Why not just leave it as it is ?
There was a problem hiding this comment.
I was unsure, but I made it throw because it would be unsafe to use this without knowing it's insensitive.
There was a problem hiding this comment.
Hmm still this seems wrong
There was a problem hiding this comment.
I can remove hashCode and equals. WDYT?
| HpackHeaderField other = (HpackHeaderField) obj; | ||
| // To avoid short circuit behavior a bitwise operator is used instead of a boolean operator. | ||
| return (HpackUtil.equalsConstantTime(name, other.name) & HpackUtil.equalsConstantTime(value, other.value)) != 0; | ||
| throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
Same... equals should never throw
|
@carl-mastrangelo do you have a benchmark by any chance? |
|
@netty-bot test this please |
|
The effect is not so pronounced, but it helps in the larger cases. My guess why insensitive is slower for small headers is that there are multiple lookups involved in the dynamic and static table. It only gets faster when the headers are already in the table. |
|
@normanmaurer I am going to send a followup PR. Disabling the huffman coder makes this ridiculously faster, to the point where this PR makes only a minor difference. |
|
@carl-mastrangelo so should we close this then ? |
|
@normanmaurer I think it will still have a minor benefit for large header values, just small headers won't see it. The two changes are independent, but the other is more important. |
|
+1
… Am 20.06.2019 um 11:03 schrieb Carl Mastrangelo ***@***.***>:
@carl-mastrangelo commented on this pull request.
In codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHeaderField.java:
> @@ -59,21 +60,16 @@ final int size() {
@OverRide
public final int hashCode() {
- // TODO(nmittler): Netty's build rules require this. Probably need a better implementation.
- return super.hashCode();
+ throw new UnsupportedOperationException();
I can remove hashCode and equals. WDYT?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I removed the two methods. I also applied this change on top of the huffman change. There is still benefit for larger header values I think. Results are a little noisy, but still give insight: |
|
@netty-bot test this please |
| @@ -145,7 +146,7 @@ static int getIndex(CharSequence name) { | |||
| * Returns the index value for the given header field in the static table. Returns -1 if the | |||
| * header field is not in the static table. | |||
| */ | |||
| static int getIndex(CharSequence name, CharSequence value) { | |||
| static int getIndexSensitive(CharSequence name, CharSequence value) { | |||
There was a problem hiding this comment.
I don't see any usage of this. And I wouldn't expect any usage since sensitive header values won't use the HPACK table.
There was a problem hiding this comment.
Yup, no use, removed.
| * Returns the lowest index value for the header field name in the dynamic table. Returns -1 if | ||
| * the header field name is not in the dynamic table. | ||
| */ | ||
| private int getIndexInsensitive(CharSequence name) { |
There was a problem hiding this comment.
How much does this buy us? I'd expect not much. Since we only pay the equals check if there is a hash collision, and these are header names which are short, it seems like there's not room for gain here.
There was a problem hiding this comment.
Maybe, but it's about code clarity. This method says it doesn't care about security, as opposed to getIndexSensitive.
Also in the case the header names change frequently, I'd expect there would be frequent collisions and (evictions).
There was a problem hiding this comment.
To me the sensitive/nonsensitive split just is a bit more confusing. I'd have preferred dumping a comment on the previous getIndex: // use constant-time comparison since used with sensitive headers. And for the value look ups // not used for sensitive headers, so no need to use constant-time comparison.
If we really like the split names, then I'd just have getIndexInsensitive() call getIndexSensitive() in the implementation.
Header name changes doesn't have anything to do with frequency of collisions. Only the number of entries plays a part in collision rate. I still maintain that for header names the difference between constant- vs variable-time comparison is well below what we should care about. I'd much rather optimize for code clarity and size here by not duplicating the getIndex implementation.
|
@netty-bot test this please |
|
@normanmaurer I would like to get this in, if there aren't any other concerns. There are always going to be differences of opinion, so we need to decide on what is a blocker and what isn't. I feel that since this code is faster, correct, and doesn't pose a significant maintainability burden, it should be committed. |
|
@carl-mastrangelo I am not strong on this... If @ejona86 is not "strongly" against it I think we can merge... @ejona86 WDYT ? |
|
Why are those both getNameIndexSensitive and getNameIndexInsensitive? It is in the name of performance, right? But there has not been any performance benefit shown from that change. Benefits are for the value change, not the name change. So the name change should be dropped. The name change adds code for no benefit. I don't care if it is renamed or the like. Avoiding the second implementation is all I'm concerned about. |
e9a2fd2 to
be363d3
Compare
|
@normanmaurer @ejona86 PTAL |
codec-http2/src/main/java/io/netty/handler/codec/http2/HpackUtil.java
Outdated
Show resolved
Hide resolved
23c6924 to
021679b
Compare
|
@normanmaurer should be ready again |
codec-http2/src/main/java/io/netty/handler/codec/http2/HpackUtil.java
Outdated
Show resolved
Hide resolved
|
I can confirm a positive impact. While on a micro level it is very little, over time it does sum up, and seems to me as a sensible optimization. |
021679b to
1fff2fb
Compare
|
@bryce-anderson can you take a look as well ? |
|
@CodingFabian thanks for confirming :) |
|
@netty-bot test this please |
codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java
Outdated
Show resolved
Hide resolved
codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java
Outdated
Show resolved
Hide resolved
955def7 to
87fc23a
Compare
Motivation: Constant time comparison functions are used to compare HTTP/2 header values, even if they are not sensitive. Modification: After checking for sensitivity, use fast comparison. Result: Faster HPACK table reads/writes
87fc23a to
87a3785
Compare
|
@bryce-anderson I added a class comment about it. @normanmaurer any chance this can make it into 4.1.43 ? |
|
@netty-bot test this please |
|
@carl-mastrangelo merged... thanks :) |
Motivation: Constant time comparison functions are used to compare HTTP/2 header values, even if they are not sensitive. Modification: After checking for sensitivity, use fast comparison. Result: Faster HPACK table reads/writes
Motivation:
Constant time comparison functions are used to compare HTTP/2 header
values, even if they are not sensitive.
Modification:
After checking for sensitivity, use fast comparison.
Result: Faster HPACK table reads/writes