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

Fix #5575 SEARCH method #5576

Merged
merged 10 commits into from Nov 11, 2020
Merged

Fix #5575 SEARCH method #5576

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
17cee2a
Fix #5575 SEARCH method
gregw Nov 5, 2020
b8f03e2
Added REPORT method
gregw Nov 5, 2020
d10b15c
+ Added all IANA methods
gregw Nov 5, 2020
f4349fa
+ Added javadoc for isSafe and isIdempotent
gregw Nov 6, 2020
064158e
+ Use enum to encapsulate fact that all safe methods are idempotent.…
gregw Nov 6, 2020
235923a
+ replaced dodgey call to toString in constructor with explicit meth…
gregw Nov 6, 2020
f1802a2
Updates from review
gregw Nov 6, 2020
3e94fae
Issue #5575 - simpler HttpMethod enum
joakime Nov 6, 2020
5e062b5
Merge pull request #5578 from eclipse/jetty-9.4.x-5575-SEARCH-smaller
joakime Nov 6, 2020
bf98ae2
Update from review
gregw Nov 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
274 changes: 152 additions & 122 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpMethod.java
View file
Open in desktop
Expand Up @@ -26,162 +26,192 @@
import org.eclipse.jetty.util.Trie;

/**
*
* Known HTTP Methods
*/
public enum HttpMethod
{
GET,
POST,
HEAD,
PUT,
OPTIONS,
DELETE,
TRACE,
CONNECT,
MOVE,
PROXY,
PRI;

/**
* Optimized lookup to find a method name and trailing space in a byte array.
*
* @param bytes Array containing ISO-8859-1 characters
* @param position The first valid index
* @param limit The first non valid index
* @return An HttpMethod if a match or null if no easy match.
*/
public static HttpMethod lookAheadGet(byte[] bytes, final int position, int limit)
// From https://www.iana.org/assignments/http-methods/http-methods.xhtml
ACL(Type.IDEMPOTENT),
BASELINE_CONTROL(Type.IDEMPOTENT),
BIND(Type.IDEMPOTENT),
CHECKIN(Type.IDEMPOTENT),
CHECKOUT(Type.IDEMPOTENT),
CONNECT(Type.NORMAL),
COPY(Type.IDEMPOTENT),
DELETE(Type.IDEMPOTENT),
GET(Type.SAFE),
HEAD(Type.SAFE),
LABEL(Type.IDEMPOTENT),
LINK(Type.IDEMPOTENT),
LOCK(Type.NORMAL),
MERGE(Type.IDEMPOTENT),
MKACTIVITY(Type.IDEMPOTENT),
MKCALENDAR(Type.IDEMPOTENT),
MKCOL(Type.IDEMPOTENT),
MKREDIRECTREF(Type.IDEMPOTENT),
MKWORKSPACE(Type.IDEMPOTENT),
MOVE(Type.IDEMPOTENT),
OPTIONS(Type.SAFE),
ORDERPATCH(Type.IDEMPOTENT),
PATCH(Type.NORMAL),
POST(Type.NORMAL),
PRI(Type.SAFE),
PROPFIND(Type.SAFE),
PROPPATCH(Type.IDEMPOTENT),
PUT(Type.IDEMPOTENT),
REBIND(Type.IDEMPOTENT),
REPORT(Type.SAFE),
SEARCH(Type.SAFE),
TRACE(Type.SAFE),
UNBIND(Type.IDEMPOTENT),
UNCHECKOUT(Type.IDEMPOTENT),
UNLINK(Type.IDEMPOTENT),
UNLOCK(Type.IDEMPOTENT),
UPDATE(Type.IDEMPOTENT),
UPDATEREDIRECTREF(Type.IDEMPOTENT),
VERSION_CONTROL(Type.IDEMPOTENT),

// Other methods
PROXY(Type.NORMAL);

// The type of the method
private enum Type
{
int length = limit - position;
if (length < 4)
return null;
switch (bytes[position])
{
case 'G':
if (bytes[position + 1] == 'E' && bytes[position + 2] == 'T' && bytes[position + 3] == ' ')
return GET;
break;
case 'P':
if (bytes[position + 1] == 'O' && bytes[position + 2] == 'S' && bytes[position + 3] == 'T' && length >= 5 && bytes[position + 4] == ' ')
return POST;
if (bytes[position + 1] == 'R' && bytes[position + 2] == 'O' && bytes[position + 3] == 'X' && length >= 6 && bytes[position + 4] == 'Y' && bytes[position + 5] == ' ')
return PROXY;
if (bytes[position + 1] == 'U' && bytes[position + 2] == 'T' && bytes[position + 3] == ' ')
return PUT;
if (bytes[position + 1] == 'R' && bytes[position + 2] == 'I' && bytes[position + 3] == ' ')
return PRI;
break;
case 'H':
if (bytes[position + 1] == 'E' && bytes[position + 2] == 'A' && bytes[position + 3] == 'D' && length >= 5 && bytes[position + 4] == ' ')
return HEAD;
break;
case 'O':
if (bytes[position + 1] == 'P' && bytes[position + 2] == 'T' && bytes[position + 3] == 'I' && length >= 8 &&
bytes[position + 4] == 'O' && bytes[position + 5] == 'N' && bytes[position + 6] == 'S' && bytes[position + 7] == ' ')
return OPTIONS;
break;
case 'D':
if (bytes[position + 1] == 'E' && bytes[position + 2] == 'L' && bytes[position + 3] == 'E' && length >= 7 &&
bytes[position + 4] == 'T' && bytes[position + 5] == 'E' && bytes[position + 6] == ' ')
return DELETE;
break;
case 'T':
if (bytes[position + 1] == 'R' && bytes[position + 2] == 'A' && bytes[position + 3] == 'C' && length >= 6 &&
bytes[position + 4] == 'E' && bytes[position + 5] == ' ')
return TRACE;
break;
case 'C':
if (bytes[position + 1] == 'O' && bytes[position + 2] == 'N' && bytes[position + 3] == 'N' && length >= 8 &&
bytes[position + 4] == 'E' && bytes[position + 5] == 'C' && bytes[position + 6] == 'T' && bytes[position + 7] == ' ')
return CONNECT;
break;
case 'M':
if (bytes[position + 1] == 'O' && bytes[position + 2] == 'V' && bytes[position + 3] == 'E' && length >= 5 && bytes[position + 4] == ' ')
return MOVE;
break;

default:
break;
}
return null;
NORMAL,
IDEMPOTENT,
SAFE
}

/**
* Optimized lookup to find a method name and trailing space in a byte array.
*
* @param buffer buffer containing ISO-8859-1 characters, it is not modified.
* @return An HttpMethod if a match or null if no easy match.
*/
public static HttpMethod lookAheadGet(ByteBuffer buffer)
{
if (buffer.hasArray())
return lookAheadGet(buffer.array(), buffer.arrayOffset() + buffer.position(), buffer.arrayOffset() + buffer.limit());
private final String _method;
private final byte[] _bytes;
private final ByteBuffer _buffer;
private final Type _type;

int l = buffer.remaining();
if (l >= 4)
{
HttpMethod m = CACHE.getBest(buffer, 0, l);
if (m != null)
{
int ml = m.asString().length();
if (l > ml && buffer.get(buffer.position() + ml) == ' ')
return m;
}
}
return null;
HttpMethod(Type type)
{
_method = name().replace('_', '-');
_type = type;
_bytes = StringUtil.getBytes(_method);
_buffer = ByteBuffer.wrap(_bytes);
}

public static final Trie<HttpMethod> INSENSITIVE_CACHE = new ArrayTrie<>();
public byte[] getBytes()
{
return _bytes;
}

static
public boolean is(String s)
{
for (HttpMethod method : HttpMethod.values())
{
INSENSITIVE_CACHE.put(method.toString(), method);
}
return toString().equalsIgnoreCase(s);
}

public static final Trie<HttpMethod> CACHE = new ArrayTernaryTrie<>(false);
/**
* An HTTP method is safe if it doesn't alter the state of the server.
* In other words, a method is safe if it leads to a read-only operation.
* Several common HTTP methods are safe: GET , HEAD , or OPTIONS .
* All safe methods are also idempotent, but not all idempotent methods are safe
* @return if the method is safe.
*/
public boolean isSafe()
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
return _type == Type.SAFE;
}

static
/**
* An idempotent HTTP method is an HTTP method that can be called many times without different outcomes.
* It would not matter if the method is called only once, or ten times over. The result should be the same.
* @return true if the method is idempotent.
*/
public boolean isIdempotent()
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
for (HttpMethod method : HttpMethod.values())
{
CACHE.put(method.toString(), method);
}
return _type.ordinal() >= Type.IDEMPOTENT.ordinal();
}

private final ByteBuffer _buffer;
private final byte[] _bytes;
public ByteBuffer asBuffer()
{
return _buffer.asReadOnlyBuffer();
}

HttpMethod()
public String asString()
{
_bytes = StringUtil.getBytes(toString());
_buffer = ByteBuffer.wrap(_bytes);
return _method;
}

public byte[] getBytes()
public String toString()
{
return _bytes;
return _method;
}

public boolean is(String s)
public static final Trie<HttpMethod> INSENSITIVE_CACHE = new ArrayTrie<>(252);
public static final Trie<HttpMethod> CACHE = new ArrayTernaryTrie<>(false, 300);
gregw marked this conversation as resolved.
Show resolved Hide resolved
public static final Trie<HttpMethod> LOOK_AHEAD = new ArrayTernaryTrie<>(false, 330);
private static final int ACL_AS_INT = ('A' & 0xff) << 24 | ('C' & 0xFF) << 16 | ('L' & 0xFF) << 8 | (' ' & 0xFF);
private static final int GET_AS_INT = ('G' & 0xff) << 24 | ('E' & 0xFF) << 16 | ('T' & 0xFF) << 8 | (' ' & 0xFF);
private static final int PRI_AS_INT = ('P' & 0xff) << 24 | ('R' & 0xFF) << 16 | ('I' & 0xFF) << 8 | (' ' & 0xFF);
private static final int PUT_AS_INT = ('P' & 0xff) << 24 | ('U' & 0xFF) << 16 | ('T' & 0xFF) << 8 | (' ' & 0xFF);
static
{
return toString().equalsIgnoreCase(s);
for (HttpMethod method : HttpMethod.values())
{
if (!INSENSITIVE_CACHE.put(method.asString(), method))
throw new IllegalStateException("INSENSITIVE_CACHE too small: " + method);

if (!CACHE.put(method.asString(), method))
throw new IllegalStateException("CACHE too small: " + method);

if (!LOOK_AHEAD.put(method.asString() + ' ', method))
throw new IllegalStateException("LOOK_AHEAD too small: " + method);
}
}

public ByteBuffer asBuffer()
/**
* Optimized lookup to find a method name and trailing space in a byte array.
*
* @param bytes Array containing ISO-8859-1 characters
* @param position The first valid index
* @param limit The first non valid index
* @return An HttpMethod if a match or null if no easy match.
* @deprecated Not used
*/
@Deprecated
public static HttpMethod lookAheadGet(byte[] bytes, final int position, int limit)
{
return _buffer.asReadOnlyBuffer();
return LOOK_AHEAD.getBest(bytes, position, limit - position);
}

public String asString()
/**
* Optimized lookup to find a method name and trailing space in a byte array.
*
* @param buffer buffer containing ISO-8859-1 characters, it is not modified.
* @return An HttpMethod if a match or null if no easy match.
*/
public static HttpMethod lookAheadGet(ByteBuffer buffer)
{
return toString();
int len = buffer.remaining();
// Short cut for 3 char methods, mostly for GET optimisation
if (len > 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really necessary?
Does this really produce any measurable performance improvement? (have you jmh'd this?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The getInt switch is an order of magnitude better. Interestingly the Map is slightly better than the Trie in this case sensitive case, but the map needs to allocate.
So doing this benchmark has convinced me I should do the getInt trick for POST and HEAD as well!

Benchmark                                                     Mode  Cnt          Score         Error   Units
HttpMethodBenchmark.testIntSwitch                            thrpt   10  234190385.517 ± 1954845.858   ops/s
HttpMethodBenchmark.testIntSwitch:·gc.alloc.rate.norm        thrpt   10         ≈ 10⁻⁵                  B/op
HttpMethodBenchmark.testMapGet                               thrpt   10   21957916.326 ±  507531.533   ops/s
HttpMethodBenchmark.testMapGet:·gc.alloc.rate.norm           thrpt   10         48.000 ±       0.001    B/op
HttpMethodBenchmark.testTrieGetBest                          thrpt   10   18475001.742 ±  205942.699   ops/s
HttpMethodBenchmark.testTrieGetBest:·gc.alloc.rate.norm      thrpt   10         ≈ 10⁻⁴                  B/op

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extending the int switch to POST and benchmarking against MOVE which doesn't have the optimisation gives:

Benchmark                                                     Mode  Cnt          Score          Error   Units
HttpMethodBenchmark.testHttpMethodMove                       thrpt   10   10435118.064 ±   240925.892   ops/s
HttpMethodBenchmark.testHttpMethodMove:·gc.alloc.rate.norm   thrpt   10         ≈ 10⁻⁴                   B/op
HttpMethodBenchmark.testHttpMethodPost                       thrpt   10  148142781.323 ± 14602351.496   ops/s
HttpMethodBenchmark.testHttpMethodPost:·gc.alloc.rate.norm   thrpt   10         ≈ 10⁻⁵                   B/op
HttpMethodBenchmark.testIntSwitch                            thrpt   10  206759099.264 ±  6371501.120   ops/s
HttpMethodBenchmark.testIntSwitch:·gc.alloc.rate.norm        thrpt   10         ≈ 10⁻⁵                   B/op
HttpMethodBenchmark.testMapGet                               thrpt   10   20527043.581 ±   415407.826   ops/s
HttpMethodBenchmark.testMapGet:·gc.alloc.rate.norm           thrpt   10         48.000 ±        0.001    B/op
HttpMethodBenchmark.testTrieGetBest                          thrpt   10   18573224.811 ±   591011.908   ops/s
HttpMethodBenchmark.testTrieGetBest:·gc.alloc.rate.norm      thrpt   10         ≈ 10⁻⁴                   B/op

So even the slightly more complex lookup for POST easily beats the non optimised MOVE by an order of magnitude.
Makes me wonder if we shouldn't make Trie (or is it now an Index) that uses getLong and getInt for most of the work!

{
switch (buffer.getInt(buffer.position()))
{
case ACL_AS_INT:
return ACL;
case GET_AS_INT:
return GET;
case PRI_AS_INT:
return PRI;
case PUT_AS_INT:
return PUT;
default:
break;
}
}
return LOOK_AHEAD.getBest(buffer, 0, len);
}

/**
* Converts the given String parameter to an HttpMethod
* Converts the given String parameter to an HttpMethod.
* The string may differ from the Enum name as a '-' in the method
* name is represented as a '_' in the Enum name.
*
* @param method the String to get the equivalent HttpMethod from
* @return the HttpMethod or null if the parameter method is unknown
Expand Down
32 changes: 26 additions & 6 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Locale;

import org.eclipse.jetty.http.HttpParser.State;
import org.eclipse.jetty.toolchain.test.Net;
Expand All @@ -32,6 +33,8 @@
import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

import static org.eclipse.jetty.http.HttpComplianceSection.NO_FIELD_FOLDING;
import static org.hamcrest.MatcherAssert.assertThat;
Expand Down Expand Up @@ -85,12 +88,20 @@ public static void parseAll(HttpParser parser, ByteBuffer buffer)
@Test
public void httpMethodTest()
{
assertNull(HttpMethod.lookAheadGet(BufferUtil.toBuffer("Wibble ")));
assertNull(HttpMethod.lookAheadGet(BufferUtil.toBuffer("GET")));
assertNull(HttpMethod.lookAheadGet(BufferUtil.toBuffer("MO")));

assertEquals(HttpMethod.GET, HttpMethod.lookAheadGet(BufferUtil.toBuffer("GET ")));
assertEquals(HttpMethod.MOVE, HttpMethod.lookAheadGet(BufferUtil.toBuffer("MOVE ")));
for (HttpMethod m : HttpMethod.values())
{
assertNull(HttpMethod.lookAheadGet(BufferUtil.toBuffer(m.asString().substring(0,2))));
assertNull(HttpMethod.lookAheadGet(BufferUtil.toBuffer(m.asString())));
assertNull(HttpMethod.lookAheadGet(BufferUtil.toBuffer(m.asString() + "FOO")));
assertEquals(m, HttpMethod.lookAheadGet(BufferUtil.toBuffer(m.asString() + " ")));
assertEquals(m, HttpMethod.lookAheadGet(BufferUtil.toBuffer(m.asString() + " /foo/bar")));

assertNull(HttpMethod.lookAheadGet(m.asString().substring(0,2).getBytes(), 0,2));
assertNull(HttpMethod.lookAheadGet(m.asString().getBytes(), 0, m.asString().length()));
assertNull(HttpMethod.lookAheadGet((m.asString() + "FOO").getBytes(), 0, m.asString().length() + 3));
assertEquals(m, HttpMethod.lookAheadGet(("\n" + m.asString() + " ").getBytes(), 1, m.asString().length() + 2));
assertEquals(m, HttpMethod.lookAheadGet(("\n" + m.asString() + " /foo").getBytes(), 1, m.asString().length() + 6));
}

ByteBuffer b = BufferUtil.allocateDirect(128);
BufferUtil.append(b, BufferUtil.toBuffer("GET"));
Expand All @@ -100,6 +111,15 @@ public void httpMethodTest()
assertEquals(HttpMethod.GET, HttpMethod.lookAheadGet(b));
}

@ParameterizedTest
@ValueSource(strings = {"GET", "POST", "VERSION-CONTROL"})
public void httpMethodNameTest(String methodName)
{
HttpMethod method = HttpMethod.fromString(methodName);
assertNotNull(method, "Method should have been found: " + methodName);
assertEquals(methodName.toUpperCase(Locale.US), method.toString());
}

@Test
public void testLineParseMockIP()
{
Expand Down
6 changes: 6 additions & 0 deletions jetty-util/src/main/java/org/eclipse/jetty/util/ArrayTernaryTrie.java
View file
Open in desktop
Expand Up @@ -368,6 +368,12 @@ public V getBest(ByteBuffer b, int offset, int len)
return getBest(0, b, offset, len);
}

@Override
public V getBest(byte[] b, int offset, int len)
{
return getBest(0, b, offset, len);
}

private V getBest(int t, byte[] b, int offset, int len)
{
int node = t;
Expand Down