Add QueryParams which deprecates HttpParameters
#2307
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: - line#1567 Modifications: - Fork our HTTP headers implementation into: - `QueryParams` - `QueryParamGetters` - `QueryParamsBase` - `QueryParamsBuilder` - .. and their implementations. - Note that it is intentional that I didn't introduce any common classes between `HttpHeaders` and `QueryParams`. They are just similar to each other accidentally and thus share nothing semantically close in common in my opinion. Please let me know if you do not like this approach - we should discuss. - Add support for `QueryParams` to annotated services. - Deprecate `HttpParameters`. Result: - Closes line#1567 To-dos: - Add encoding/decoding methods like we did for `Cookie`. - Update `annotated-services.rst`
|
I agree with the idea of not sharing when they are semantically different. But it's just so so much code... I'm not a fan but I see why we need to. I guess we do want to redefine the API since indeed they are separate concepts but can we at least share code for the storage implementation? I would be ok with just one extra object allocation to have storage as a separate class, or it might be ok to use a base class. |
|
@anuraaga Will try. 😅 |
Codecov Report
@@ Coverage Diff @@
## master #2307 +/- ##
===========================================
+ Coverage 73.62% 73.7% +0.08%
- Complexity 10182 10314 +132
===========================================
Files 885 894 +9
Lines 38950 39346 +396
Branches 4818 4891 +73
===========================================
+ Hits 28677 29001 +324
- Misses 7816 7864 +48
- Partials 2457 2481 +24
Continue to review full report at Codecov.
|
|
anuraaga
reviewed
Dec 12, 2019
core/src/main/java/com/linecorp/armeria/common/HttpHeadersBase.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/StringMultimap.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/QueryParamsBase.java
Outdated
Show resolved
Hide resolved
ikhoon
approved these changes
Dec 13, 2019
core/src/main/java/com/linecorp/armeria/common/StringMultimapGetters.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/QueryParams.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/StringMultimap.java
Outdated
Show resolved
Hide resolved
|
The result from fd1c4e5 |
|
Slightly better numbers after 8383ef9. Now we are on par with or better than Guava except for one case (ASCII). |
anuraaga
approved these changes
Dec 19, 2019
core/src/main/java/com/linecorp/armeria/common/QueryStringDecoder.java
Outdated
Show resolved
Hide resolved
|
|
||
| @SuppressWarnings("checkstyle:AvoidEscapedUnicodeCharacters") | ||
| private static final char UNKNOWN_CHAR = '\uFFFD'; | ||
| private static final byte[] OCTETS_TO_HEX = new byte['f' + 1]; |
There was a problem hiding this comment.
I may stick with int[] since the difference in size is so minor, but it could give the JVM a better time of finding some optimization.
There was a problem hiding this comment.
I'm not sure. I'd prefer byte[] because it's more compact and thus better utilization of CPU cache.
core/src/main/java/com/linecorp/armeria/common/QueryStringEncoder.java
Outdated
Show resolved
Hide resolved
|
Beware stack allocations of arrays ie https://mobile.twitter.com/forked_franz/status/1193481376122777601 |
|
Result from: 1cb66cb |
|
@franz1981 wrote:
Thanks for the information. Does it mean JVM will not stack-allocate any array whose length is derived from user input? e.g. static void func(int length) {
final int values[] = new int[length];
// ... `values` does not escape ...
} |
|
@trustin I need to re-read the heuristic of the EA, but looking at the simple JMH bench I've sent, yes: you are both limited on the array size by a JVM params and it seems OpenJDK (!= GraalVM) is more conservative, so I suggest to run your benchmarks with |
minwoox
approved these changes
Dec 20, 2019
benchmarks/src/jmh/java/com/linecorp/armeria/common/QueryStringDecoderBenchmark.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/QueryParams.java
Outdated
Show resolved
Hide resolved
|
Thanks a lot, @franz1981 Complete result from 1cb66cb with 10 forks: |
|
Thanks everyone for reviews and advices. 👍 |
fmguerreiro
pushed a commit
to fmguerreiro/armeria
that referenced
this pull request
Sep 19, 2020
Motivation: - line#1567 Modifications: - Fork our HTTP headers API into: - `QueryParams` - `QueryParamGetters` - `QueryParamsBase` - `QueryParamsBuilder` - Extract the common logic and API between HTTP API and query params API into: - `StringMultimap` - `StringMultimapGetters` - Add support for `QueryParams` to annotated services. - Update documentation. - Deprecate `HttpParameters`. - Replace the usage of Netty `QueryString{Encoder,Decoder}` with: - `QueryParamGetter.toQueryString()` and `appendQueryString()` - `QueryParams.fromQueryString()` Result: - Closes line#1567 - Nicer and faster way to decode/encode a query string. ``` Benchmark Mode Cnt Score Error Units QueryStringDecoderBenchmark.armeriaAscii thrpt 50 378741.654 ± 6659.789 ops/s QueryStringDecoderBenchmark.armeriaLong thrpt 50 17363.007 ± 711.308 ops/s QueryStringDecoderBenchmark.armeriaMixed thrpt 50 120362.631 ± 2517.298 ops/s QueryStringDecoderBenchmark.armeriaUnicode thrpt 50 86819.473 ± 1580.938 ops/s QueryStringDecoderBenchmark.nettyAscii thrpt 50 352954.120 ± 1303.213 ops/s QueryStringDecoderBenchmark.nettyLong thrpt 50 9216.124 ± 285.652 ops/s QueryStringDecoderBenchmark.nettyMixed thrpt 50 62038.399 ± 453.026 ops/s QueryStringDecoderBenchmark.nettyUnicode thrpt 50 49561.678 ± 659.775 ops/s QueryStringEncoderBenchmark.armeriaAscii thrpt 50 802149.965 ± 4023.009 ops/s QueryStringEncoderBenchmark.armeriaLong thrpt 50 21525.055 ± 801.401 ops/s QueryStringEncoderBenchmark.armeriaMixed thrpt 50 224702.833 ± 1475.840 ops/s QueryStringEncoderBenchmark.armeriaUnicode thrpt 50 210494.132 ± 8571.742 ops/s QueryStringEncoderBenchmark.guavaAscii thrpt 50 774172.127 ± 4041.075 ops/s QueryStringEncoderBenchmark.guavaLong thrpt 50 22669.871 ± 184.624 ops/s QueryStringEncoderBenchmark.guavaMixed thrpt 50 199361.612 ± 1562.677 ops/s QueryStringEncoderBenchmark.guavaUnicode thrpt 50 139650.766 ± 991.154 ops/s QueryStringEncoderBenchmark.jdkAscii thrpt 50 265860.618 ± 511.369 ops/s QueryStringEncoderBenchmark.jdkLong thrpt 50 7932.388 ± 59.079 ops/s QueryStringEncoderBenchmark.jdkMixed thrpt 50 64110.415 ± 299.827 ops/s QueryStringEncoderBenchmark.jdkUnicode thrpt 50 51915.861 ± 145.560 ops/s QueryStringEncoderBenchmark.nettyAscii thrpt 50 438574.293 ± 8949.848 ops/s QueryStringEncoderBenchmark.nettyLong thrpt 50 14510.677 ± 159.799 ops/s QueryStringEncoderBenchmark.nettyMixed thrpt 50 126900.454 ± 1168.554 ops/s QueryStringEncoderBenchmark.nettyUnicode thrpt 50 101262.449 ± 1587.243 ops/s ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Modifications:
QueryParamsQueryParamGettersQueryParamsBaseQueryParamsBuilderStringMultimapStringMultimapGettersQueryParamsto annotated services.HttpParameters.QueryString{Encoder,Decoder}with:QueryParamGetter.toQueryString()andappendQueryString()QueryParams.fromQueryString()QueryString{Encoder,Decoder}.Result: