Fix #5575 SEARCH method

[gregw]

Fix #5575 SEARCH and PATCH methods

[gregw]

@lorban note that we have "manual" tries as well!

[gregw]

Hmmm perhaps we should do all known methods and just use a Trie:

ACL	no	yes	[RFC3744, Section 8.1]
BASELINE-CONTROL	no	yes	[RFC3253, Section 12.6]
BIND	no	yes	[RFC5842, Section 4]
CHECKIN	no	yes	[RFC3253, Section 4.4, Section 9.4]
CHECKOUT	no	yes	[RFC3253, Section 4.3, Section 8.8]
CONNECT	no	no	[RFC7231, Section 4.3.6]
COPY	no	yes	[RFC4918, Section 9.8]
DELETE	no	yes	[RFC7231, Section 4.3.5]
GET	yes	yes	[RFC7231, Section 4.3.1]
HEAD	yes	yes	[RFC7231, Section 4.3.2]
LABEL	no	yes	[RFC3253, Section 8.2]
LINK	no	yes	[RFC2068, Section 19.6.1.2]
LOCK	no	no	[RFC4918, Section 9.10]
MERGE	no	yes	[RFC3253, Section 11.2]
MKACTIVITY	no	yes	[RFC3253, Section 13.5]
MKCALENDAR	no	yes	[RFC4791, Section 5.3.1][RFC8144, Section 2.3]
MKCOL	no	yes	[RFC4918, Section 9.3][RFC5689, Section 3][RFC8144, Section 2.3]
MKREDIRECTREF	no	yes	[RFC4437, Section 6]
MKWORKSPACE	no	yes	[RFC3253, Section 6.3]
MOVE	no	yes	[RFC4918, Section 9.9]
OPTIONS	yes	yes	[RFC7231, Section 4.3.7]
ORDERPATCH	no	yes	[RFC3648, Section 7]
PATCH	no	no	[RFC5789, Section 2]
POST	no	no	[RFC7231, Section 4.3.3]
PRI	yes	yes	[RFC7540, Section 3.5]
PROPFIND	yes	yes	[RFC4918, Section 9.1][RFC8144, Section 2.1]
PROPPATCH	no	yes	[RFC4918, Section 9.2][RFC8144, Section 2.2]
PUT	no	yes	[RFC7231, Section 4.3.4]
REBIND	no	yes	[RFC5842, Section 6]
REPORT	yes	yes	[RFC3253, Section 3.6][RFC8144, Section 2.1]
SEARCH	yes	yes	[RFC5323, Section 2]
TRACE	yes	yes	[RFC7231, Section 4.3.8]
UNBIND	no	yes	[RFC5842, Section 5]
UNCHECKOUT	no	yes	[RFC3253, Section 4.5]
UNLINK	no	yes	[RFC2068, Section 19.6.1.3]
UNLOCK	no	yes	[RFC4918, Section 9.11]
UPDATE	no	yes	[RFC3253, Section 7.1]
UPDATEREDIRECTREF	no	yes	[RFC4437, Section 7]
VERSION-CONTROL	no	yes	[RFC3253, Section 3.5]

[gregw]

@lorban if you want to make this part of your Trie cleanup and implement all of those methods above, then just take the test, make a new CACHE and close this PR.

[gregw]

Note that list is from http://www.iana.org/assignments/http-methods/http-methods.xhtml

[joakime]

Reduce the amount of work you are doing by using enum features properly.

[gregw]

@joakime @lorban nudge

[joakime]

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

[gregw]

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

[gregw]

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!

[lorban]

Like @joakime I wonder if those shortcuts in lookAheadGet are worth it.

[gregw]

@joakime @lorban whilst a Trie based on getInt and/or getLong is possible, it will be a very difficult class to implement and will be rather space intensive. To implement, we'd need to build a tree of Nodes where each Node had:

• a Map<Long, Node> to do 8 byte look aheads
• a Map<Int, Node> to do 4 byte look aheads if the 8 byte branches missed
• A normal Trie to do <4 byte look aheads if both 8 byte and 4 byte look aheads missed.

I think this could be very memory intensive, as even something simple like HttpMethod would result in have 20 or 30 Maps in its tree. Perhaps it is something we could look at if ever a Trie lookup was seen in the a flamegraph.

So for now, I think having a simple optimisation for GET and POST is a reasonable thing to do as it can be done with minimal complexity. Note that we have for a very long time had a hand coded optimisation for direct matching of common methods, so this is not adding an optimisation, but simply changing it's implementation to be a little better whilst handling more methods in general.

[gregw]

@joakime nudge