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.

Sign up for GitHub

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

Jump to bottom

Jetty 12 - Fix for HttpParser quick HTTP Version lookahead #9171

Merged
merged 15 commits into from
Jan 17, 2023
Merged

Jetty 12 - Fix for HttpParser quick HTTP Version lookahead #9171

merged 15 commits into from
Jan 17, 2023

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Jan 13, 2023

Fixes #9170

@joakime joakime added this to the 12.0.x milestone Jan 13, 2023
@joakime joakime requested review from gregw, sbordet and lorban January 13, 2023 22:50
@joakime joakime self-assigned this Jan 13, 2023
@joakime joakime linked an issue Jan 13, 2023 that may be closed by this pull request
Jetty 12 - HttpParser Version optimization never works #9170
Closed
@joakime joakime mentioned this pull request Jan 13, 2023
Closed
@gregw
Further improve fix.
593cfdb 
+ Removed the array optimized look ahead
+ Added caches for version follow by eoln and space
+ version cache hits now include following character and skip state transition.
+ improved test harness to check lf and crlf endings
@gregw
Copy link
Contributor

gregw commented Jan 16, 2023

@joakime I further improved this fix and pushed 593cfdb to your branch:

  • Removed the array optimized look ahead
  • Added caches for version follow by eoln and space
  • version cache hits now include following character and skip state transition.
  • improved test harness to check lf and crlf endings

gregw added 4 commits January 16, 2023 13:04
@gregw
Further improve fix.
94753a7 
+ Removed the array optimized look ahead
+ Added caches for version follow by eoln and space
+ version cache hits now include following character and skip state transition.
+ improved test harness to check lf and crlf endings
@gregw
Further improve fix.
d9ee9f5 
+ Removed the array optimized look ahead
+ Added caches for version follow by eoln and space
+ version cache hits now include following character and skip state transition.
+ improved test harness to check lf and crlf endings
@gregw
Further improve fix.
ceef61c 
+ Removed the array optimized look ahead
+ Added caches for version follow by eoln and space
+ version cache hits now include following character and skip state transition.
+ improved test harness to check lf and crlf endings
@gregw
Further improve fix.
aa2a263 
+ Removed the array optimized look ahead
+ Added caches for version follow by eoln and space
+ version cache hits now include following character and skip state transition.
+ improved test harness to check lf and crlf endings
+ added HTTP as int optimization
@gregw
Copy link
Contributor

gregw commented Jan 16, 2023

Added another optimization to check for HTTP as a single get of an int.

gregw added 2 commits January 16, 2023 16:11
@gregw
Further improve fix.
17ea5e9 
+ Removed the array optimized look ahead
+ Added caches for version follow by eoln and space
+ version cache hits now include following character and skip state transition.
+ improved test harness to check lf and crlf endings
+ added HTTP as int optimization
@gregw
Further improve fix.
84ebfe4 
+ Removed the array optimized look ahead
+ version lookahead now done with getInt
+ improved test harness to check lf and crlf endings
+ added HTTP as int optimization
+ fixed bug in near miss of field cache
@gregw
Copy link
Contributor

gregw commented Jan 16, 2023

@joakime I found a few more issues with the field cache, plus I went a simpler route for the version lookahead using getInt

joakime
joakime commented Jan 16, 2023
View reviewed changes
Copy link
Contributor Author

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not every place uses eoln

jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java Show resolved Hide resolved
jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java Show resolved Hide resolved
jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java Show resolved Hide resolved
jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java Outdated Show resolved Hide resolved
@joakime
Copy link
Contributor Author

joakime commented Jan 16, 2023

You introduced a parameterized eoln, but it's not used in all test cases.

sbordet
sbordet requested changes Jan 16, 2023
View reviewed changes
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java Outdated
if (_version != null)
int v = buffer.getInt(position + 4);
_version = v == HTTP_1_1_AS_INT ? HttpVersion.HTTP_1_1 : v == HTTP_1_0_AS_INT ? HttpVersion.HTTP_1_0 : null;
if (_version != null && buffer.get(position + 8) == ' ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it quite hard to read and reason about the buffer with these "magic" numbers, like 9, 4, 8, etc.
Can it be rewritten using a int current to indicate where in the buffer we are looking at?
Something like:

else if (_responseHandler != null && buffer.remaining() >= 9)
{
    int current = position;
    int h = buffer.getInt(current);
    if (h == HTTP_AS_INT)
    {
        current += 4;
        int v = buffer.getInt(current);
        ...
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some constants. Is that clearer?

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpTokens.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java Outdated
Comment on lines 1280 to 1289
int pos = buffer.position() + n.length() + (v == null ? 0 : v.length()) + 1;
if (v == null || pos >= buffer.limit())
{
// Header only
setState(FieldState.VALUE);
_string.setLength(0);
_length = 0;
buffer.position(buffer.position() + n.length() + 1);
buffer.position(v == null ? pos : pos - v.length());
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not feel right.
pos may be set past the cached value, and we may still enter the if block due to pos >= buffer.limit(), but then we set the state to be VALUE?

Even if it is correct it's just way more difficult to read, so perhaps needs comments and/or examples.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restructured to posAfterName and posAfterValue... but doing some more checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more cleanup and testing done. OK now.

@gregw gregw requested a review from sbordet January 16, 2023 23:34
@gregw gregw requested a review from sbordet January 17, 2023 21:47
@gregw gregw merged commit a06d1a2 into jetty-12.0.x Jan 17, 2023
@gregw gregw deleted the fix/jetty-12.0.x/httpparser-version-getbest-optimization branch January 17, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@gregw gregw Awaiting requested review from gregw

@lorban lorban Awaiting requested review from lorban

Assignees

@joakime joakime

Labels
Performance
Projects
None yet
Milestone
12.0.x
Development

Successfully merging this pull request may close these issues.

Jetty 12 - HttpParser Version optimization never works
3 participants
@joakime @gregw @sbordet