HttpObjectDecoder: Create methods for HttpContent and LastHttpContent #4097
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: We've removed access to the activeStreams collection, we should do the same for the children of a stream to provide a consistent interface. Modifications: Moved Http2StreamVisitor to a top-level interface. Removed unnecessary child operations from the Http2Stream interface so that we no longer require a map structure. Result: Cleaner and more consistent interface for iterating over child streams.
… for CipherSuiteFilter Motivation: To support HTTP2 we need APLN support. This was not provided before when using OpenSslEngine, so SSLEngine (JDK one) was the only bet. Beside this CipherSuiteFilter was not supported Modifications: - Upgrade netty-tcnative and make use of new features to support ALPN and NPN in server and client mode. - Guard against segfaults after the ssl pointer is freed - support correctly different failure behaviours - add support for CipherSuiteFilter Result: Be able to use OpenSslEngine for ALPN / NPN for server and client.
Motivation: Now that we have a CharObjectHashMap, we should change Http2Settings to use it. Modifications: Changed Http2Settings to extend CharObjectHashMap rather than IntObjectHashMap. Result: Http2Settings uses less memory to store keys.
Motiviation:
At the moment we use FIFO for the PoolThreadCache which is sub-optimal as this may reduce the changes to have the cached memory actual still in the cpu-cache.
Modification:
- Change to use LIFO as this increase the chance to be able to serve buffers from the cpu-cache
Results:
Faster allocation out of the ThreadLocal cache.
Before the commit:
[xxx wrk]$ ./wrk -H 'Connection: keep-alive' -d 120 -c 256 -t 16 -s scripts/pipeline-many.lua http://xxx:8080/plaintext
Running 2m test @ http://xxx:8080/plaintext
16 threads and 256 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 14.69ms 10.06ms 131.43ms 80.10%
Req/Sec 283.89k 40.37k 433.69k 66.81%
533859742 requests in 2.00m, 72.09GB read
Requests/sec: 4449510.51
Transfer/sec: 615.29MB
After the commit:
[xxx wrk]$ ./wrk -H 'Connection: keep-alive' -d 120 -c 256 -t 16 -s scripts/pipeline-many.lua http://xxx:8080/plaintext
Running 2m test @ http://xxx:8080/plaintext
16 threads and 256 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 16.38ms 26.32ms 734.06ms 97.38%
Req/Sec 283.86k 39.31k 361.69k 83.38%
540836511 requests in 2.00m, 73.04GB read
Requests/sec: 4508150.18
Transfer/sec: 623.40MB
Motivation: We missed to flush the channel when using HttpChunkedInput (this is done when using SSL). This will result in a stale. Modifications: Replace ctx.write(...) with ctx.writeAndFlush(...) Result: Correctly working example.
Motivation: We missed to dereference the chunk and tmpNioBuf when calling deallocate(). This means the GC can not collect these as we still hold a reference while have the PooledByteBuf in the recycler stack. Modifications: Dereference chunk and tmpNioBuf. Result: GC can collect things.
This reverts commit 7094c7b.
Motivation: The ReplayingDecoderBuffer does not match the naming scheme we use for ByteBuf types. Modifications: Rename to ReplayingDecoderByteBuf to match naming scheme Result: Consistent naming
Motivation: CompositeByteBuf has an iterator() method but fails to implement Iterable Modifications: Let CompositeByteBuf implement Iterable<ByteBuf> Result: Easier usage
…hreadCache Motivation: When create NormalMemoryRegionCache for PoolThreadCache, we overbooked cache array size. This means unnecessary overhead for thread local cache as we will create multi cache enties for each element in cache array. Modifications: change: int arraySize = Math.max(1, max / area.pageSize); to: int arraySize = Math.max(1, log2(max / area.pageSize) + 1); Result: Now arraySize won't introduce unnecessary overhead. Changes to be committed: modified: buffer/src/main/java/io/netty/buffer/PoolThreadCache.java
Motivation: The DefaultHttp2Connection is not checking for RuntimeExceptions when invoking Http2Connection.Listener methods. This is a problem for a few reasons: 1. The state of DefaultHttp2Connection will be corrupted if a listener throws a RuntimeException. 2. If the first listener throws then no other listeners will be notified, which may further corrupt state that is updated as a result of listeners being notified. Modifications: - Document that RuntimeExceptions are not supported for Http2Connection.Listener methods, and will be logged as an error. - Update DefaultHttp2Connection to handle and exception for each listener that is notified, and be sure that 1 listener throwing an exception does not prevent others from being notified. Result: More robust DefaultHttp2Connection.
Motivation: The Http2FrameWriterBenchmark JMH harness class name was not updated for the JVM arguments. The number of forks is 0 which means the JHM will share a JVM with the benchmarks. Sharing the JVM may lead to less reliable benchmarking results and as doesn't allow for the command line arguments to be applied for each benchmark. Modifications: - Update the JMH version from 0.9 to 1.7.1. Benchmarks wouldn't run on old version. - Increase the number of forks from 0 to 1. - Remove allocation of environment from static and cleanup AfterClass to using the Setup and Teardown methods. The forked JVM would not shut down correctly otherwise (and wait for 30+ seconds before timeing out). Result: Benchmarks that run as intended.
Motivation: Due to a recent flurry of cleanup and fixes, we no longer need the stream removal policy to protect against recently removed streams. We should get rid of it. Modifications: Removed Http2StreamRemovalPolicy and everywhere it's used. Result: Fixes netty#3448
Motivation: It needs to be fast :) Modifications: Added a simple benchmark to the microbench module. Result: Yay, benchmarks!
Motivation: As we missed to correctly handle EPOLLRDHUP we produce an IOException which is unnessary. This leads to have exceptionCaught(...) methods called. Modifications: When EPOLLRDHUP was received just close the socket and fail all pending writes. Result: Correctly handle of EPOLLRDHUP and so not miss-leading exceptions.
Motivation: SslContext factory methods have gotten out of control; it's past time to swap to a builder. Modifications: New Builder class. The existing factory methods must be left as-is for backward compatibility. Result: Fixes netty#3531
Motivation: Sometimes it's useful to use EC keys and not DSA or RSA. We should support it. Modifications: Support EC keys and share the code between JDK and Openssl impl. Result: It's possible to use EC keys now.
Motivation: Attribute.getAndRemove() will return the value but also remove the AttributeKey itself from the AttributeMap. This may not what you want as you may want to keep an instance of it and just set it later again. Document the contract so the user know what to expect. Modifications: - Make it clear when to use AttributeKey.getAndRemove() / AttributeKey.remove() and when AttributeKey.getAndSet(null) / AttributeKey.set(null). Result: Less suprising behaviour.
Motivation: The usage and code within AsciiString has exceeded the original design scope for this class. Its usage as a binary string is confusing and on the verge of violating interface assumptions in some spots. Modifications: - ByteString will be created as a base class to AsciiString. All of the generic byte handling processing will live in ByteString and all the special character encoding will live in AsciiString. Results: The AsciiString interface will be clarified. Users of AsciiString can now be clear of the limitations the class imposes while users of the ByteString class don't have to live with those limitations.
Motivation: While forward porting netty#3579 there were a few areas that had not been previously back ported. Modifications: Backport the missed areas to ensure consistency. Result: More consistent 4.1 and master branches.
Motivation: If an exclusive dependency change stream B should be an exclusive dependency of stream A is requested and stream B is already a child of stream A...then we will add B to B's own children map and create a circular link in the priority tree. This leads to an infinite recursive loop and a stack overflow exception. Modifications: -when removeAllChildren is called it should not remove the exclusive dependency. -unit test to ensure this case is covered. Result: No more circular link in the priority tree.
Motivation: Other implementations of FullHttpMessage allow .toString to be called after the Message has been released This brings AggregatedFullHttpMessage into line with those impls. Modifications: - Changed AggregatedFullHttpMessage to no longer be a sub-class of DefaultByteBufHolder - Changes AggregatedFullHttpMessage to implement ByteBufHolder - Hold the content buffer internally to AggregatedFullHttpMessage - Implement the required content() and release() methods that were missing - Do not check refcnt when accessing content() (similar to DefaultFullHttpMessage) Result: A released AggregatedFullHttpMessage can have .toString called without throwing an exception
Motivation: Class.forName() documents that null will use bootstrap loader: http://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#forName-java.lang.String-boolean-java.lang.ClassLoader- But the link between "null" and bootstrap loader is even more explicit in ClassLoader's documentation: http://docs.oracle.com/javase/8/docs/api/java/lang/ClassLoader.html#getParent-- The current code is trying to use the bootstrap loader but seems to have not been aware of the meaning of null. Modifications: Use "null" as the class loader when we want to load classes in the bootstrap loader. Result: More reliable ALPN/NPN loading and simpler code.
Motivation: The IntObjectHashMap benchmarks show the Agrona collections to be faster on put, lookup, and remove. One major difference is that we're using 2 modulus operations each time we increment the position index while iterating. Agrona uses a mask instead. Modifications: Modified the KObjectHashMap to use masking rather than modulus when wrapping the position index. This requires that the capacity be a power of 2. Result: Improved performance of IntObjectHashMap.
Motivation: ByteString#hashCode() trashes its own hash code if it's being accessed concurrently Modifications: Pull the ByteString#hash into a local variable and calculate it locally. Result: ByteString#hashCode() is no longer returning a junk value.
Motivation: The current implementation does byte by byte comparison, which we have seen can be a performance bottleneck when the AsciiString is used as the key in a Map. Modifications: Use sun.misc.Unsafe (on supporting platforms) to compare up to eight bytes at a time and get closer to the performance of String.equals(Object). Result: Significant improvement (2x - 6x) in performance over the current implementation. Benchmark (size) Mode Samples Score Score error Units i.n.m.i.PlatformDependentBenchmark.arraysBytesEqual 10 thrpt 10 118843477.518 2347259.347 ops/s i.n.m.i.PlatformDependentBenchmark.arraysBytesEqual 50 thrpt 10 43910319.773 198376.996 ops/s i.n.m.i.PlatformDependentBenchmark.arraysBytesEqual 100 thrpt 10 26339969.001 159599.252 ops/s i.n.m.i.PlatformDependentBenchmark.arraysBytesEqual 1000 thrpt 10 2873119.030 20779.056 ops/s i.n.m.i.PlatformDependentBenchmark.arraysBytesEqual 10000 thrpt 10 306370.450 1933.303 ops/s i.n.m.i.PlatformDependentBenchmark.arraysBytesEqual 100000 thrpt 10 25750.415 108.391 ops/s i.n.m.i.PlatformDependentBenchmark.unsafeBytesEqual 10 thrpt 10 248077563.510 635320.093 ops/s i.n.m.i.PlatformDependentBenchmark.unsafeBytesEqual 50 thrpt 10 128198943.138 614827.548 ops/s i.n.m.i.PlatformDependentBenchmark.unsafeBytesEqual 100 thrpt 10 86195621.349 1063959.307 ops/s i.n.m.i.PlatformDependentBenchmark.unsafeBytesEqual 1000 thrpt 10 16920264.598 61615.365 ops/s i.n.m.i.PlatformDependentBenchmark.unsafeBytesEqual 10000 thrpt 10 1687454.747 6367.602 ops/s i.n.m.i.PlatformDependentBenchmark.unsafeBytesEqual 100000 thrpt 10 153717.851 586.916 ops/s
…scellaneous issues. Motivation: Allows for running benchmarks from built jars which is useful in development environments that only take released artifacts. Modifications: Move benchmarks into 'main' from 'test' Add @State annotations to benchmarks that are missing them Fix timing issue grabbing context during channel initialization Result: Users can run benchmarks more easily.
Motivation: There is no benchmark to measure the priority tree implementation performance. Modifications: Introduce a new benchmark which will populate the priority tree, and then shuffle parent/child links around. Result: A simple benchmark to get a baseline for the HTTP/2 codec's priority tree implementation.
Motivation: We should support XXXCollections methods for all primitive map types. Modifications: Removed PrimitiveCollections and added a template for XXXCollections. Result: Fixes netty#4001
Motivation: When looking through the logs for entries pertaining to a specific stream, it's difficult because header entries use the syntax "streamId:<id>" but all other entries use "streamId=<id>". We should make all of the entries consistent. Modifications: Changed header entries to use "streamId=<id>" to match the other entries. Result: Easier HTTP/2 log navigation.
Motivation: Two problems: 1. Decoder assumption that as soon as it finds </ element it can decrement opened xml brackets counter. It can lead to bugs when closing bracket is not in byteBuf yet. 2. Not proper handling of more than two root elements in XML document. First element will be processed properly, second one not. It is caused by assumption that byteBuf readerIndex is 0 at the begging of decoding. Modifications: Both problems were resolved by fixes: 1. decrement opened brackets count only if </ > enclosing bracket is found 2. consider readerIndex higher than 0 when counting output frame length Result: Both problems were resolved
…Pool Motivation: We don't decrease acquired channel count in FixedChannelPool when timeout occurs by AcquireTimeoutAction.NEW and eventually fails. Modifications: Set AcquireTask.acquired=true to call decrementAndRunTaskQueue when timeout action fails. Result: Acquired channel count decreases correctly.
Motivation: The HttpObjectDecoder is on the hot code path for the http codec. There are a few hot methods which can be modified to improve performance. Modifications: - Modify AppendableCharSequence to provide unsafe methods which don't need to re-check bounds for every call. - Update HttpObjectDecoder methods to take advantage of new AppendableCharSequence methods. Result: Peformance boost for decoding http objects.
Motivation: As per http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718033 Modification: Add isSessionPresent() method and constructor to set it Result: Be able to use it with mqtt 3.1.1
Motivation: The MqttEncoder was failing to build because it was using a method that doesn't exist. Modifications: Change sessionPresent() to isSessionPresent(). Result: MqttEnccoder is now able to build.
Motivation: IP_FREEBIND allows to bind to addresses without the address up yet or even the interface configured yet. Modifications: Add support for IP_FREEBIND. Result: It's now possible to use IP_FREEBIND when using the native epoll transport.
Motivation: The SPDY spec requires that all header names be lowercase (see https://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3-1#TOC-3.2-HTTP-Request-Response). The SPDY codec header name validator does not enforce this requirement. Modifications: - SpdyCodecUtil.validateHeaderName should check for upper case characters and throw an error if any are found. Result: SPDY codec header validation enforces specification requirement.
Motivation: The DataCompressionHttp2Test was exiting prematurely leading to unit test failures. Modifications: - Fix the race condition so the test does not evaluate final conditions until all expected events occur Result: Unit test no longer fails
…SPYD. Fixes netty#3600 Motivation: We noticed that the headers implementation in Netty for HTTP/2 uses quite a lot of memory and that also at least the performance of randomly accessing a header is quite poor. The main concern however was memory usage, as profiling has shown that a DefaultHttp2Headers not only use a lot of memory it also wastes a lot due to the underlying hashmaps having to be resized potentially several times as new headers are being inserted. This is tracked as issue netty#3600. Modifications: We redesigned the DefaultHeaders to simply take a Map object in its constructor and reimplemented the class using only the Map primitives. That way the implementation is very concise and hopefully easy to understand and it allows each concrete headers implementation to provide its own map or to even use a different headers implementation for processing requests and writing responses i.e. incoming headers need to provide fast random access while outgoing headers need fast insertion and fast iteration. The new implementation can support this with hardly any code changes. It also comes with the advantage that if the Netty project decides to add a third party collections library as a dependency, one can simply plug in one of those very fast and memory efficient map implementations and get faster and smaller headers for free. For now, we are using the JDK's TreeMap for HTTP and HTTP/2 default headers. Result: - Significantly fewer lines of code in the implementation. While the total commit is still roughly 400 lines less, the actual implementation is a lot less. I just added some more tests and microbenchmarks. - Overall performance is up. The current implementation should be significantly faster for insertion and retrieval. However, it is slower when it comes to iteration. There is simply no way a TreeMap can have the same iteration performance as a linked list (as used in the current headers implementation). That's totally fine though, because when looking at the benchmark results @ejona86 pointed out that the performance of the headers is completely dominated by insertion, that is insertion is so significantly faster in the new implementation that it does make up for several times the iteration speed. You can't iterate what you haven't inserted. I am demonstrating that in this spreadsheet [1]. (Actually, iteration performance is only down for HTTP, it's significantly improved for HTTP/2). - Memory is down. The implementation with TreeMap uses on avg ~30% less memory. It also does not produce any garbage while being resized. In load tests for GRPC we have seen a memory reduction of up to 1.2KB per RPC. I summarized the memory improvements in this spreadsheet [1]. The data was generated by [2] using JOL. - While it was my original intend to only improve the memory usage for HTTP/2, it should be similarly improved for HTTP, SPDY and STOMP as they all share a common implementation. [1] https://docs.google.com/spreadsheets/d/1ck3RQklyzEcCLlyJoqDXPCWRGVUuS-ArZf0etSXLVDQ/edit#gid=0 [2] https://gist.github.com/buchgr/4458a8bdb51dd58c82b4
Motivation: In the event an HTTP message does not include either a content-length or a transfer-encoding header [RFC 7230](https://tools.ietf.org/html/rfc7230#section-3.3.3) states the behavior must be treated differently for requests and responses. If the channel is half closed then the HttpObjectDecoder is not invoking decodeLast and thus not checking if messages should be sent up the pipeline. Modifications: - Add comments to clarify regular decode default case. - Handle the ChannelInputShutdownEvent in the HttpObjectDecoder and evaluate if messages need to be generated. Result: Messages are generated on half closed, and comments clarify existing logic.
Motiviation: The current read loops don't fascilitate reading a maximum amount of bytes. This capability is useful to have more fine grain control over how much data is injested. Modifications: - Add a setMaxBytesPerRead(int) and getMaxBytesPerRead() to ChannelConfig - Add a setMaxBytesPerIndividualRead(int) and getMaxBytesPerIndividualRead to ChannelConfig - Add methods to RecvByteBufAllocator so that a pluggable scheme can be used to control the behavior of the read loop. - Modify read loop for all transport types to respect the new RecvByteBufAllocator API Result: The ability to control how many bytes are read for each read operation/loop, and a more extensible read loop.
Motivation: Even though MemoryRegionCache$Entry instances are allocated through a recycler they are not properly recycled, leaving a lot of instances to be GCed along with Recycler$DefaultHandle objects. Fixes netty#4071 Modification: Recycle Entry when done using it. Result: Less GCed objects.
Motivation: The acquire channel function resulted in calling itself several times in case when channel polled from the pool queue was unhealthy, which resulted FixedChannelPool to be called several times which in it's turn caused FixedChannelPool.acquire() to be called and resulted into acquireChannelCount to be unnecessary increased. Example use case: 1) Create FixedChannelPool instance with one channel in the pool: new FixedChannelPool(cb, handler, 1) 2) Acquire channel A from the pool 3) close the channel A 4) Return it back to the pool 5) Acquire channel from the same pool again Expected result: new channel created and acquired, channel A that has been closed discarded and removed from the pool from being unhealthy Actual result: Channel A had been removed from the pool, how ever the new channel had never be acquired, instead the request to acquire had been added to the pending queue in FixedChannelPool and the acquireChannelCount is increased by one. The reason is that at the time when SimpleChannelPool figured out that the channel was unhealthy called FixedChannelPool.acquire to try to acquire new channel, how ever the request was added to the pendingTakQueue because by the time when FixedChannelPool.acquire was called, the acquireChannelCount was already "1" so new channel ould not be created cause of maxChannelsLimit=1. Modifications: The suggested approach modifies the SimpleChannelPool in a way so that when channel detected to be unhealthy it calls private method SimpleChannelPool.acquireHealthyFromPoolOrNew() which guarantees that SimpleChannelPool actually either finds a healthy channel in the pool and returns it or causes the promise.cause() in case when new channel was failed to be created. Result: The ```acquiredChannelCount``` is now calculated correctly as a result of SimpleChannelPool.acquire() of not being recursive on overridable acquire method.
Motivation: Sometimes the user already has a PrivateKey / X509Certificate which should be used to create a new SslContext. At the moment we only allow to construct it via Files. Modifications: - Add new methods to the SslContextBuilder to allow creating a SslContext from PrivateKey / X509Certificate - Mark all public constructors of *SslContext as @deprecated, the user should use SslContextBuilder - Update tests to us SslContextBuilder. Result: Creating of SslContext is possible with PrivateKay/X509Certificate
Motivation: We not set any optimization flag when compile native transport Modification: Add -O3 to CFLAGS to have GCC do optimizations Result: Ship optimized native code
Motivation: The Http2ConnectionHandler was writing pending bytes, but was not flushing. This may result in deadlock. Modifications: - Http2ConnectionHandler must writePendingBytes and also flush. Result: Data is now flushed after writabilityChange writes more data to underlying layers.
Motivation: We provide a hyperlink to the docs for SPDY if the runtime is not setup correctly to help users. These docs have moved. Modifications: - Update the hyperlink to point to the new doc location. Result: Users are able to find docs more easily.
Motivation: New versions of alpn-boot and npn-boot have been released. Modifications: - Update pom to pull in new versions. Result: Dependencies more up to date.
Motivation: If is enabled and a channel is half closed it is possible for the EPOLL event loop to get into an infinite loop by continuously being woken up on the EPOLLRDHUP event. Modifications: - Ensure that the EPOLLRDHUP event is unregistered for to prevent infinite loop. Result: 1 less infinite loop.
…Decoder Motivation: Due not using a cast we insert 32 and not a whitespace into the String. Modifications: Correclty cast to char. Result: Correct handling of whitespaces.
Motivation: It keeps failing on some of our CI machines for an unknown reason. Modifications: Ignore it temporarily Result: Less noise in CI result
…n the HttpObjectDecoder and create() methods in the HttpObjectAggregator. netty#4060 Motivation: This change allows us to implement higher level HTTP features such as "request scopes" or "session scopes" by being able to customize the HttpMessage and share state that spans HttpMessage over HttpContent to LastHttpContent. Modifications: The HttpObjectDecoder was changed to use explcit create() methods for HttpContent and LastHttpContent objects. The HttpObjectAggregator was changed to expose its internal "aggregated" message types and two create() methods were added. Result: It's now possible to customize the message types that HttpObjectDecoder return and possibly share state that spans from HttpMessage, over HttpContent to LastHttpContent and effectivly implement higher level HTTP abstractions such as "request scopes" or "session scopes".
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.
See #4060