-
Notifications
You must be signed in to change notification settings - Fork 3.9k
core: speed up Status code and message parsing #2238
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.
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
Conversation
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
@grpc-jenkins ok to test |
983850b
to
210e226
Compare
Updated the code to show how to share this across the io.grpc.internal boundary. The status line parser also shows up in the allocation profiler. |
eca4dd4
to
4c8b3bd
Compare
One last thing: The difference here is noticeable:
I included the full numbers in the second commit message, but this better. The 99% latency goes down by 25us! |
@carl-mastrangelo wow! very nice! 6% faster! I ll have a look at it tomorrow morning . it's almost midnight here 😴 :) |
looking now ... sorry for the delay ... |
* the gRPC team first. | ||
*/ | ||
@Internal | ||
public final class InternalMetadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI Our internal visibility rules center mostly around files, so restricted actions can be put in a file that can be protected in ways the java type system can't. Open source doesn't have this, so they just have to read the comments.
4c8b3bd
to
9dbade4
Compare
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
*/ | ||
|
||
package io.grpc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not put this in the internal package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this has to be in the public package as a bridge between internal and public. It accesses the package protected methods in io.grpc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that was only necessary if it accessed public methods in io.grpc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. The Metadata.Key.of
is package protected and has a special implementation for TrustedAsciiKey (which pairs with TrustedAsciiMarshaller in this class). io.grpc.internal
needs access to this package because of HTTP_STATUS_MARSHALLER (as well as others).
Finally, all the shortcutting work between transport headers and Metadata are going to be don in this class.
Also, all the other
LGTM, once remaining comments are addressed and build passes. Good work @carl-mastrangelo ! |
Fixed the fallthrough |
This introduces the idea of a "Trusted" Ascii Marshaller, which is known to always produce valid ASCII byte arrays. This saves a surprising amount of garbage, since String conversion involves creating a new java.lang.StringCoding, and a sun.nio.cs.US_ASCII. There are other types that can be converted (notably Http2ClientStream's :status marshaller, which is particularly wasteful). Before: Benchmark Mode Cnt Score Error Units StatusBenchmark.codeDecode sample 641278 88.889 ± 9.673 ns/op StatusBenchmark.codeEncode sample 430800 73.014 ± 1.444 ns/op StatusBenchmark.messageDecodeEscape sample 433467 441.078 ± 58.373 ns/op StatusBenchmark.messageDecodePlain sample 676526 268.620 ± 7.849 ns/op StatusBenchmark.messageEncodeEscape sample 547350 1211.243 ± 29.907 ns/op StatusBenchmark.messageEncodePlain sample 419318 223.263 ± 9.673 ns/op After: Benchmark Mode Cnt Score Error Units StatusBenchmark.codeDecode sample 442241 48.310 ± 2.409 ns/op StatusBenchmark.codeEncode sample 622026 35.475 ± 0.642 ns/op StatusBenchmark.messageDecodeEscape sample 595572 312.407 ± 15.870 ns/op StatusBenchmark.messageDecodePlain sample 565581 99.090 ± 8.799 ns/op StatusBenchmark.messageEncodeEscape sample 479147 201.422 ± 10.765 ns/op StatusBenchmark.messageEncodePlain sample 560957 94.722 ± 1.187 ns/op Also fixes grpc#2237 Before: Result "unaryCall1024": mean = 155710.268 ±(99.9%) 149.278 ns/op Percentiles, ns/op: p(0.0000) = 63552.000 ns/op p(50.0000) = 151552.000 ns/op p(90.0000) = 188672.000 ns/op p(95.0000) = 207360.000 ns/op p(99.0000) = 260608.000 ns/op p(99.9000) = 358912.000 ns/op p(99.9900) = 1851425.792 ns/op p(99.9990) = 11161178.767 ns/op p(99.9999) = 14985005.383 ns/op p(100.0000) = 17235968.000 ns/op Benchmark (direct) (transport) Mode Cnt Score Error Units TransportBenchmark.unaryCall1024 true NETTY sample 3205966 155710.268 ± 149.278 ns/op After: Result "unaryCall1024": mean = 147474.794 ±(99.9%) 128.733 ns/op Percentiles, ns/op: p(0.0000) = 59520.000 ns/op p(50.0000) = 144640.000 ns/op p(90.0000) = 176128.000 ns/op p(95.0000) = 190464.000 ns/op p(99.0000) = 236544.000 ns/op p(99.9000) = 314880.000 ns/op p(99.9900) = 1113084.723 ns/op p(99.9990) = 10783126.979 ns/op p(99.9999) = 13887153.242 ns/op p(100.0000) = 15253504.000 ns/op Benchmark (direct) (transport) Mode Cnt Score Error Units TransportBenchmark.unaryCall1024 true NETTY sample 3385015 147474.794 ± 128.733 ns/op
e16289a
to
1623063
Compare
Thanks for taking the time @buchgr ! |
This introduces the idea of a "Trusted" Ascii Marshaller, which is
known to always produce valid ASCII byte arrays. This saves a
surprising amount of garbage, since String conversion involves
creating a new java.lang.StringCoding, and a sun.nio.cs.US_ASCII.
There are other types that can be converted (notably
Http2ClientStream's :status marshaller, which is particularly
wasteful).
Before:
After:
Also fixes #2237
cc: @ejona86 for the fairly complex parsing logic.