From 3a58063fe7b9fc8eb4e1c5a096bbe0680d4b2c7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Br=C3=A9gier?= Date: Thu, 19 Nov 2020 08:00:35 +0100 Subject: [PATCH] Fix for performance regression on HttpPost RequestDecoder (#10623) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix issue #10508 where PARANOID mode slow down about 1000 times compared to ADVANCED. Also fix a rare issue when internal buffer was growing over a limit, it was partially discarded using `discardReadBytes()` which causes bad changes within previously discovered HttpData. Reasons were: Too many `readByte()` method calls while other ways exist (such as keep in memory the last scan position when trying to find a delimiter or using `bytesBefore(firstByte)` instead of looping externally). Changes done: - major change on way buffer are parsed: instead of read byte per byte until found delimiter, try to find the delimiter using `bytesBefore()` and keep the last unfound position to skeep already parsed parts (algorithms are the same but implementation of scan are different) - Change the condition to discard read bytes when refCnt is at most 1. Observations using Async-Profiler: ================================== 1) Without optimizations, most of the time (more than 95%) is through `readByte()` method within `loadDataMultipartStandard` method. 2) With using `bytesBefore(byte)` instead of `readByte()` to find various delimiter, the `loadDataMultipartStandard` method is going down to 19 to 33% depending on the test used. the `readByte()` method or equivalent `getByte(pos)` method are going down to 15% (from 95%). Times are confirming those profiling: - With optimizations, in SIMPLE mode about 82% better, in ADVANCED mode about 79% better and in PARANOID mode about 99% better (most of the duplicate read accesses are removed or make internally through `bytesBefore(byte)` method) A benchmark is added to show the behavior of the various cases (one big item, such as File upload, and many items) and various level of detection (Disabled, Simple, Advanced, Paranoid). This benchmark is intend to alert if new implementations make too many differences (such as the previous version where about PARANOID gives about 1000 times slower than other levels, while it is now about at most 10 times). Extract of Benchmark run: ========================= Run complete. Total time: 00:13:27 Benchmark Mode Cnt Score Error Units HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigAdvancedLevel thrpt 6 2,248 ± 0,198 ops/ms HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigDisabledLevel thrpt 6 2,067 ± 1,219 ops/ms HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigParanoidLevel thrpt 6 1,109 ± 0,038 ops/ms HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigSimpleLevel thrpt 6 2,326 ± 0,314 ops/ms HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighAdvancedLevel thrpt 6 1,444 ± 0,226 ops/ms HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighDisabledLevel thrpt 6 1,462 ± 0,642 ops/ms HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighParanoidLevel thrpt 6 0,159 ± 0,003 ops/ms HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighSimpleLevel thrpt 6 1,522 ± 0,049 ops/ms --- .../HttpPostMultipartRequestDecoder.java | 475 ++++++------------ .../HttpPostStandardRequestDecoder.java | 9 +- .../multipart/HttpPostRequestDecoderTest.java | 31 +- ...pPostMultipartRequestDecoderBenchmark.java | 202 ++++++++ .../codec/http/multipart/package-info.java | 19 + 5 files changed, 391 insertions(+), 345 deletions(-) create mode 100644 microbench/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoderBenchmark.java create mode 100644 microbench/src/main/java/io/netty/handler/codec/http/multipart/package-info.java diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java index 1eda4ed00d..38cba72466 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java @@ -128,6 +128,11 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest */ private Attribute currentAttribute; + /** + * The current Data position before finding delimiter + */ + private int lastDataPosition; + private boolean destroyed; private int discardThreshold = HttpPostRequestDecoder.DEFAULT_DISCARD_THRESHOLD; @@ -342,12 +347,15 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest // which is not really usable for us as we may exceed it once we add more bytes. buf.alloc().buffer(buf.readableBytes()).writeBytes(buf); } else { + int readPos = undecodedChunk.readerIndex(); + int writable = undecodedChunk.writableBytes(); + int toWrite = buf.readableBytes(); + if (undecodedChunk.refCnt() == 1 && writable < toWrite && readPos + writable >= toWrite) { + undecodedChunk.discardReadBytes(); + } undecodedChunk.writeBytes(buf); } parseBody(); - if (undecodedChunk != null && undecodedChunk.writerIndex() > discardThreshold) { - undecodedChunk.discardReadBytes(); - } return this; } @@ -965,47 +973,6 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest currentFieldAttributes.remove(HttpHeaderValues.FILENAME); } - /** - * Read one line up to the CRLF or LF - * - * @return the String from one line - * @throws NotEnoughDataDecoderException - * Need more chunks and reset the {@code readerIndex} to the previous - * value - */ - private static String readLineStandard(ByteBuf undecodedChunk, Charset charset) { - int readerIndex = undecodedChunk.readerIndex(); - ByteBuf line = undecodedChunk.alloc().heapBuffer(64); - try { - while (undecodedChunk.isReadable()) { - byte nextByte = undecodedChunk.readByte(); - if (nextByte == HttpConstants.CR) { - // check but do not changed readerIndex - nextByte = undecodedChunk.getByte(undecodedChunk.readerIndex()); - if (nextByte == HttpConstants.LF) { - // force read - undecodedChunk.readByte(); - return line.toString(charset); - } else { - // Write CR (not followed by LF) - line.writeByte(HttpConstants.CR); - } - } else if (nextByte == HttpConstants.LF) { - return line.toString(charset); - } else { - line.writeByte(nextByte); - } - } - } catch (IndexOutOfBoundsException e) { - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(e); - } finally { - line.release(); - } - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); - } - /** * Read one line up to the CRLF or LF * @@ -1015,44 +982,23 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest * value */ private static String readLine(ByteBuf undecodedChunk, Charset charset) { - if (!undecodedChunk.hasArray()) { - return readLineStandard(undecodedChunk, charset); + final int readerIndex = undecodedChunk.readerIndex(); + int posLf = undecodedChunk.bytesBefore(HttpConstants.LF); + if (posLf == -1) { + throw new NotEnoughDataDecoderException(); } - SeekAheadOptimize sao = new SeekAheadOptimize(undecodedChunk); - int readerIndex = undecodedChunk.readerIndex(); - ByteBuf line = undecodedChunk.alloc().heapBuffer(64); - try { - while (sao.pos < sao.limit) { - byte nextByte = sao.bytes[sao.pos++]; - if (nextByte == HttpConstants.CR) { - if (sao.pos < sao.limit) { - nextByte = sao.bytes[sao.pos++]; - if (nextByte == HttpConstants.LF) { - sao.setReadPosition(0); - return line.toString(charset); - } else { - // Write CR (not followed by LF) - sao.pos--; - line.writeByte(HttpConstants.CR); - } - } else { - line.writeByte(nextByte); - } - } else if (nextByte == HttpConstants.LF) { - sao.setReadPosition(0); - return line.toString(charset); - } else { - line.writeByte(nextByte); - } - } - } catch (IndexOutOfBoundsException e) { - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(e); - } finally { - line.release(); + boolean crFound = + undecodedChunk.getByte(readerIndex + posLf - 1) == HttpConstants.CR; + if (crFound) { + posLf--; } - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); + CharSequence line = undecodedChunk.readCharSequence(posLf, charset); + if (crFound) { + undecodedChunk.skipBytes(2); + } else { + undecodedChunk.skipBytes(1); + } + return line.toString(); } /** @@ -1070,201 +1016,73 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest * Need more chunks and reset the {@code readerIndex} to the previous * value */ - private static String readDelimiterStandard(ByteBuf undecodedChunk, String delimiter) { - int readerIndex = undecodedChunk.readerIndex(); + private String readDelimiter(ByteBuf undecodedChunk, String delimiter) { + final int readerIndex = undecodedChunk.readerIndex(); try { - StringBuilder sb = new StringBuilder(64); - int delimiterPos = 0; - int len = delimiter.length(); - while (undecodedChunk.isReadable() && delimiterPos < len) { - byte nextByte = undecodedChunk.readByte(); - if (nextByte == delimiter.charAt(delimiterPos)) { - delimiterPos++; - sb.append((char) nextByte); + final int len = delimiter.length(); + if (len + 2 > undecodedChunk.readableBytes()) { + // Not able to check if "--" is present + throw new NotEnoughDataDecoderException(); + } + int newPositionDelimiter = findDelimiter(undecodedChunk, delimiter, 0); + if (newPositionDelimiter != 0) { + // Delimiter not fully found + throw new NotEnoughDataDecoderException(); + } + byte nextByte = undecodedChunk.getByte(readerIndex + len); + // first check for opening delimiter + if (nextByte == HttpConstants.CR) { + nextByte = undecodedChunk.getByte(readerIndex + len + 1); + if (nextByte == HttpConstants.LF) { + CharSequence line = undecodedChunk.readCharSequence(len, charset); + undecodedChunk.skipBytes(2); + return line.toString(); } else { + // error since CR must be followed by LF // delimiter not found so break here ! undecodedChunk.readerIndex(readerIndex); throw new NotEnoughDataDecoderException(); } - } - // Now check if either opening delimiter or closing delimiter - if (undecodedChunk.isReadable()) { - byte nextByte = undecodedChunk.readByte(); - // first check for opening delimiter - if (nextByte == HttpConstants.CR) { - nextByte = undecodedChunk.readByte(); - if (nextByte == HttpConstants.LF) { - return sb.toString(); - } else { - // error since CR must be followed by LF - // delimiter not found so break here ! - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); - } - } else if (nextByte == HttpConstants.LF) { - return sb.toString(); - } else if (nextByte == '-') { - sb.append('-'); - // second check for closing delimiter - nextByte = undecodedChunk.readByte(); - if (nextByte == '-') { - sb.append('-'); - // now try to find if CRLF or LF there - if (undecodedChunk.isReadable()) { + } else if (nextByte == HttpConstants.LF) { + CharSequence line = undecodedChunk.readCharSequence(len, charset); + undecodedChunk.skipBytes(1); + return line.toString(); + } else if (nextByte == '-') { + // second check for closing delimiter + nextByte = undecodedChunk.getByte(readerIndex + len + 1); + if (nextByte == '-') { + CharSequence line = undecodedChunk.readCharSequence(len + 2, charset); + // now try to find if CRLF or LF there + if (undecodedChunk.isReadable()) { + nextByte = undecodedChunk.readByte(); + if (nextByte == HttpConstants.CR) { nextByte = undecodedChunk.readByte(); - if (nextByte == HttpConstants.CR) { - nextByte = undecodedChunk.readByte(); - if (nextByte == HttpConstants.LF) { - return sb.toString(); - } else { - // error CR without LF - // delimiter not found so break here ! - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); - } - } else if (nextByte == HttpConstants.LF) { - return sb.toString(); + if (nextByte == HttpConstants.LF) { + return line.toString(); } else { - // No CRLF but ok however (Adobe Flash uploader) - // minus 1 since we read one char ahead but - // should not - undecodedChunk.readerIndex(undecodedChunk.readerIndex() - 1); - return sb.toString(); + // error CR without LF + // delimiter not found so break here ! + undecodedChunk.readerIndex(readerIndex); + throw new NotEnoughDataDecoderException(); } - } - // FIXME what do we do here? - // either considering it is fine, either waiting for - // more data to come? - // lets try considering it is fine... - return sb.toString(); - } - // only one '-' => not enough - // whatever now => error since incomplete - } - } - } catch (IndexOutOfBoundsException e) { - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(e); - } - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); - } - - /** - * Read one line up to --delimiter or --delimiter-- and if existing the CRLF - * or LF. Note that CRLF or LF are mandatory for opening delimiter - * (--delimiter) but not for closing delimiter (--delimiter--) since some - * clients does not include CRLF in this case. - * - * @param delimiter - * of the form --string, such that '--' is already included - * @return the String from one line as the delimiter searched (opening or - * closing) - * @throws NotEnoughDataDecoderException - * Need more chunks and reset the readerInder to the previous - * value - */ - private static String readDelimiter(ByteBuf undecodedChunk, String delimiter) { - if (!undecodedChunk.hasArray()) { - return readDelimiterStandard(undecodedChunk, delimiter); - } - SeekAheadOptimize sao = new SeekAheadOptimize(undecodedChunk); - int readerIndex = undecodedChunk.readerIndex(); - int delimiterPos = 0; - int len = delimiter.length(); - try { - StringBuilder sb = new StringBuilder(64); - // check conformity with delimiter - while (sao.pos < sao.limit && delimiterPos < len) { - byte nextByte = sao.bytes[sao.pos++]; - if (nextByte == delimiter.charAt(delimiterPos)) { - delimiterPos++; - sb.append((char) nextByte); - } else { - // delimiter not found so break here ! - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); - } - } - // Now check if either opening delimiter or closing delimiter - if (sao.pos < sao.limit) { - byte nextByte = sao.bytes[sao.pos++]; - if (nextByte == HttpConstants.CR) { - // first check for opening delimiter - if (sao.pos < sao.limit) { - nextByte = sao.bytes[sao.pos++]; - if (nextByte == HttpConstants.LF) { - sao.setReadPosition(0); - return sb.toString(); + } else if (nextByte == HttpConstants.LF) { + return line.toString(); } else { - // error CR without LF - // delimiter not found so break here ! - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); + // No CRLF but ok however (Adobe Flash uploader) + // minus 1 since we read one char ahead but + // should not + undecodedChunk.readerIndex(undecodedChunk.readerIndex() - 1); + return line.toString(); } - } else { - // error since CR must be followed by LF - // delimiter not found so break here ! - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); - } - } else if (nextByte == HttpConstants.LF) { - // same first check for opening delimiter where LF used with - // no CR - sao.setReadPosition(0); - return sb.toString(); - } else if (nextByte == '-') { - sb.append('-'); - // second check for closing delimiter - if (sao.pos < sao.limit) { - nextByte = sao.bytes[sao.pos++]; - if (nextByte == '-') { - sb.append('-'); - // now try to find if CRLF or LF there - if (sao.pos < sao.limit) { - nextByte = sao.bytes[sao.pos++]; - if (nextByte == HttpConstants.CR) { - if (sao.pos < sao.limit) { - nextByte = sao.bytes[sao.pos++]; - if (nextByte == HttpConstants.LF) { - sao.setReadPosition(0); - return sb.toString(); - } else { - // error CR without LF - // delimiter not found so break here ! - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); - } - } else { - // error CR without LF - // delimiter not found so break here ! - undecodedChunk.readerIndex(readerIndex); - throw new NotEnoughDataDecoderException(); - } - } else if (nextByte == HttpConstants.LF) { - sao.setReadPosition(0); - return sb.toString(); - } else { - // No CRLF but ok however (Adobe Flash - // uploader) - // minus 1 since we read one char ahead but - // should not - sao.setReadPosition(1); - return sb.toString(); - } - } - // FIXME what do we do here? - // either considering it is fine, either waiting for - // more data to come? - // lets try considering it is fine... - sao.setReadPosition(0); - return sb.toString(); - } - // whatever now => error since incomplete - // only one '-' => not enough or whatever not enough - // element } + // FIXME what do we do here? + // either considering it is fine, either waiting for + // more data to come? + // lets try considering it is fine... + return line.toString(); } + // only one '-' => not enough + // whatever now => error since incomplete } } catch (IndexOutOfBoundsException e) { undecodedChunk.readerIndex(readerIndex); @@ -1275,96 +1093,93 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest } /** - * Load the field value or file data from a Multipart request + * @param undecodedChunk the source where the delimiter is to be found + * @param delimiter the string to find out + * @param offset the offset from readerIndex within the undecodedChunk to + * start from to find out the delimiter * - * @return {@code true} if the last chunk is loaded (boundary delimiter found), {@code false} if need more chunks - * @throws ErrorDataDecoderException + * @return a number >= 0 if found, else new offset with negative value + * (to inverse), both from readerIndex + * @throws NotEnoughDataDecoderException + * Need more chunks while relative position with readerIndex is 0 */ - private static boolean loadDataMultipartStandard(ByteBuf undecodedChunk, String delimiter, HttpData httpData) { + private static int findDelimiter(ByteBuf undecodedChunk, String delimiter, int offset) { final int startReaderIndex = undecodedChunk.readerIndex(); final int delimeterLength = delimiter.length(); - int index = 0; - int lastPosition = startReaderIndex; - byte prevByte = HttpConstants.LF; - boolean delimiterFound = false; - while (undecodedChunk.isReadable()) { - final byte nextByte = undecodedChunk.readByte(); - // Check the delimiter - if (prevByte == HttpConstants.LF && nextByte == delimiter.codePointAt(index)) { - index++; - if (delimeterLength == index) { - delimiterFound = true; + final int toRead = undecodedChunk.readableBytes(); + int newOffset = offset; + boolean delimiterNotFound = true; + while (delimiterNotFound && newOffset + delimeterLength <= toRead) { + int posFirstChar = undecodedChunk + .bytesBefore(startReaderIndex + newOffset, toRead - newOffset, + (byte) delimiter.codePointAt(0)); + if (posFirstChar == -1) { + newOffset = toRead; + return -newOffset; + } + newOffset = posFirstChar + offset; + if (newOffset + delimeterLength > toRead) { + return -newOffset; + } + // assume will found it + delimiterNotFound = false; + for (int index = 1; index < delimeterLength; index++) { + if (undecodedChunk.getByte(startReaderIndex + newOffset + index) != delimiter.codePointAt(index)) { + // ignore first found offset and redo search from next char + newOffset++; + delimiterNotFound = true; break; } - continue; } - lastPosition = undecodedChunk.readerIndex(); - if (nextByte == HttpConstants.LF) { - index = 0; - lastPosition -= (prevByte == HttpConstants.CR)? 2 : 1; + } + if (delimiterNotFound || newOffset + delimeterLength > toRead) { + if (newOffset == 0) { + throw new NotEnoughDataDecoderException(); } - prevByte = nextByte; + return -newOffset; } - if (prevByte == HttpConstants.CR) { - lastPosition--; - } - ByteBuf content = undecodedChunk.retainedSlice(startReaderIndex, lastPosition - startReaderIndex); - try { - httpData.addContent(content, delimiterFound); - } catch (IOException e) { - throw new ErrorDataDecoderException(e); - } - undecodedChunk.readerIndex(lastPosition); - return delimiterFound; + return newOffset; } /** * Load the field value from a Multipart request * * @return {@code true} if the last chunk is loaded (boundary delimiter found), {@code false} if need more chunks + * * @throws ErrorDataDecoderException */ - private static boolean loadDataMultipart(ByteBuf undecodedChunk, String delimiter, HttpData httpData) { - if (!undecodedChunk.hasArray()) { - return loadDataMultipartStandard(undecodedChunk, delimiter, httpData); - } - final SeekAheadOptimize sao = new SeekAheadOptimize(undecodedChunk); + private boolean loadDataMultipart(ByteBuf undecodedChunk, String delimiter, + HttpData httpData) { final int startReaderIndex = undecodedChunk.readerIndex(); - final int delimeterLength = delimiter.length(); - int index = 0; - int lastRealPos = sao.pos; - byte prevByte = HttpConstants.LF; - boolean delimiterFound = false; - while (sao.pos < sao.limit) { - final byte nextByte = sao.bytes[sao.pos++]; - // Check the delimiter - if (prevByte == HttpConstants.LF && nextByte == delimiter.codePointAt(index)) { - index++; - if (delimeterLength == index) { - delimiterFound = true; - break; - } - continue; - } - lastRealPos = sao.pos; - if (nextByte == HttpConstants.LF) { - index = 0; - lastRealPos -= (prevByte == HttpConstants.CR)? 2 : 1; - } - prevByte = nextByte; - } - if (prevByte == HttpConstants.CR) { - lastRealPos--; - } - final int lastPosition = sao.getReadPosition(lastRealPos); - final ByteBuf content = undecodedChunk.retainedSlice(startReaderIndex, lastPosition - startReaderIndex); + int newOffset; try { - httpData.addContent(content, delimiterFound); + newOffset = findDelimiter(undecodedChunk, delimiter, lastDataPosition); + if (newOffset < 0) { + // delimiter not found + lastDataPosition = -newOffset; + return false; + } + } catch (NotEnoughDataDecoderException e) { + // Not enough data and no change to lastDataPosition + return false; + } + // found delimiter but still need to check if CRLF before + int startDelimiter = newOffset; + if (undecodedChunk.getByte(startReaderIndex + startDelimiter - 1) == HttpConstants.LF) { + startDelimiter--; + if (undecodedChunk.getByte(startReaderIndex + startDelimiter - 1) == HttpConstants.CR) { + startDelimiter--; + } + } + ByteBuf content = undecodedChunk.retainedSlice(startReaderIndex, startDelimiter); + try { + httpData.addContent(content, true); } catch (IOException e) { throw new ErrorDataDecoderException(e); } - undecodedChunk.readerIndex(lastPosition); - return delimiterFound; + lastDataPosition = 0; + undecodedChunk.readerIndex(startReaderIndex + startDelimiter); + return true; } /** diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java index bec61ae8b4..429e5bbbad 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java @@ -300,12 +300,15 @@ public class HttpPostStandardRequestDecoder implements InterfaceHttpPostRequestD // which is not really usable for us as we may exceed it once we add more bytes. buf.alloc().buffer(buf.readableBytes()).writeBytes(buf); } else { + int readPos = undecodedChunk.readerIndex(); + int writable = undecodedChunk.writableBytes(); + int toWrite = buf.readableBytes(); + if (undecodedChunk.refCnt() == 1 && writable < toWrite && readPos + writable >= toWrite) { + undecodedChunk.discardReadBytes(); + } undecodedChunk.writeBytes(buf); } parseBody(); - if (undecodedChunk != null && undecodedChunk.writerIndex() > discardThreshold) { - undecodedChunk.discardReadBytes(); - } return this; } diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java index ba01e4d240..45b8461ce7 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java @@ -132,6 +132,7 @@ public class HttpPostRequestDecoderTest { // Create decoder instance to test. final HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(inMemoryFactory, req); assertFalse(decoder.getBodyHttpDatas().isEmpty()); + req.release(); decoder.destroy(); } @@ -178,6 +179,7 @@ public class HttpPostRequestDecoderTest { assertNotNull(datar); assertEquals(datas[i].getBytes(CharsetUtil.UTF_8).length, datar.length); + req.release(); decoder.destroy(); } } @@ -211,6 +213,7 @@ public class HttpPostRequestDecoderTest { // Create decoder instance to test. final HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(inMemoryFactory, req); assertFalse(decoder.getBodyHttpDatas().isEmpty()); + req.release(); decoder.destroy(); } @@ -368,6 +371,7 @@ public class HttpPostRequestDecoderTest { // Create decoder instance to test. final HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(inMemoryFactory, req); assertFalse(decoder.getBodyHttpDatas().isEmpty()); + req.release(); decoder.destroy(); } @@ -397,6 +401,7 @@ public class HttpPostRequestDecoderTest { assertTrue(part1 instanceof FileUpload); FileUpload fileUpload = (FileUpload) part1; assertEquals("tmp 0.txt", fileUpload.getFilename()); + req.release(); decoder.destroy(); } @@ -427,6 +432,7 @@ public class HttpPostRequestDecoderTest { // Create decoder instance to test without any exception. final HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(inMemoryFactory, req); assertFalse(decoder.getBodyHttpDatas().isEmpty()); + req.release(); decoder.destroy(); } @@ -459,8 +465,8 @@ public class HttpPostRequestDecoderTest { FileUpload fileUpload = (FileUpload) part1; byte[] fileBytes = fileUpload.get(); assertTrue("the filecontent should not be decoded", filecontent.equals(new String(fileBytes))); - decoder.destroy(); req.release(); + decoder.destroy(); } @Test @@ -538,8 +544,8 @@ public class HttpPostRequestDecoderTest { } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) { assertTrue(e.getCause() instanceof IllegalArgumentException); } finally { - decoder.destroy(); content.release(); + decoder.destroy(); } } @@ -573,8 +579,8 @@ public class HttpPostRequestDecoderTest { assertTrue("the item should be a FileUpload", part1 instanceof FileUpload); FileUpload fileUpload = (FileUpload) part1; assertEquals("the filename should be decoded", filename, fileUpload.getFilename()); - decoder.destroy(); req.release(); + decoder.destroy(); } // https://github.com/netty/netty/pull/7265 @@ -609,8 +615,8 @@ public class HttpPostRequestDecoderTest { assertTrue("the item should be a FileUpload", part1 instanceof FileUpload); FileUpload fileUpload = (FileUpload) part1; assertEquals("the filename should be decoded", filename, fileUpload.getFilename()); - decoder.destroy(); req.release(); + decoder.destroy(); } // https://github.com/netty/netty/pull/7265 @@ -704,6 +710,7 @@ public class HttpPostRequestDecoderTest { assertTrue(part1 instanceof FileUpload); FileUpload fileUpload = (FileUpload) part1; assertEquals("tmp-0.txt", fileUpload.getFilename()); + req.release(); decoder.destroy(); } @@ -752,7 +759,7 @@ public class HttpPostRequestDecoderTest { FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", Unpooled.copiedBuffer("a=1&&b=2", CharsetUtil.US_ASCII)); try { - new HttpPostStandardRequestDecoder(request); + new HttpPostStandardRequestDecoder(request).destroy(); } finally { assertTrue(request.release()); } @@ -772,7 +779,7 @@ public class HttpPostRequestDecoderTest { buf.writeCharSequence("a=b&foo=%22bar%22&==", CharsetUtil.US_ASCII); FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", buf); try { - new HttpPostStandardRequestDecoder(request); + new HttpPostStandardRequestDecoder(request).destroy(); } finally { assertTrue(request.release()); } @@ -823,8 +830,8 @@ public class HttpPostRequestDecoderTest { FileUpload fileUpload = (FileUpload) part1; assertEquals("the filename should be decoded", filename, fileUpload.getFilename()); - decoder.destroy(); req.release(); + decoder.destroy(); } @Test @@ -860,8 +867,8 @@ public class HttpPostRequestDecoderTest { assertTrue(attr.getByteBuf().isDirect()); assertEquals("los angeles", attr.getValue()); - decoder.destroy(); req.release(); + decoder.destroy(); } @Test @@ -877,7 +884,7 @@ public class HttpPostRequestDecoderTest { } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) { assertEquals("Invalid hex byte at index '0' in string: '%'", e.getMessage()); } finally { - req.release(); + assertTrue(req.release()); } } @@ -894,7 +901,7 @@ public class HttpPostRequestDecoderTest { } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) { assertEquals("Invalid hex byte at index '0' in string: '%2'", e.getMessage()); } finally { - req.release(); + assertTrue(req.release()); } } @@ -911,7 +918,7 @@ public class HttpPostRequestDecoderTest { } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) { assertEquals("Invalid hex byte at index '0' in string: '%Zc'", e.getMessage()); } finally { - req.release(); + assertTrue(req.release()); } } @@ -928,7 +935,7 @@ public class HttpPostRequestDecoderTest { } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) { assertEquals("Invalid hex byte at index '0' in string: '%2g'", e.getMessage()); } finally { - req.release(); + assertTrue(req.release()); } } } diff --git a/microbench/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoderBenchmark.java b/microbench/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoderBenchmark.java new file mode 100644 index 0000000000..c570cd6745 --- /dev/null +++ b/microbench/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoderBenchmark.java @@ -0,0 +1,202 @@ +/* + * Copyright 2020 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package io.netty.handler.codec.http.multipart; + +import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; +import io.netty.handler.codec.http.DefaultHttpContent; +import io.netty.handler.codec.http.DefaultHttpRequest; +import io.netty.handler.codec.http.DefaultLastHttpContent; +import io.netty.handler.codec.http.HttpHeaderNames; +import io.netty.handler.codec.http.HttpMethod; +import io.netty.handler.codec.http.HttpVersion; +import io.netty.microbench.util.AbstractMicrobenchmark; +import io.netty.util.ResourceLeakDetector; +import io.netty.util.ResourceLeakDetector.Level; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +import java.util.concurrent.TimeUnit; + + +@Threads(1) +@Warmup(iterations = 2) +@Measurement(iterations = 3) +@OutputTimeUnit(TimeUnit.MILLISECONDS) +public class HttpPostMultipartRequestDecoderBenchmark + extends AbstractMicrobenchmark { + + public double testHighNumberChunks(boolean big, boolean noDisk) { + String BOUNDARY = "01f136d9282f"; + int size = 8 * 1024; + int chunkNumber = 64; + StringBuilder stringBuilder = new StringBuilder(size); + stringBuilder.setLength(size); + String data = stringBuilder.toString(); + + byte[] bodyStartBytes = ("--" + BOUNDARY + "\n" + + "Content-Disposition: form-data; name=\"msg_id\"\n\n15200\n--" + + BOUNDARY + + "\nContent-Disposition: form-data; name=\"msg1\"; filename=\"file1.txt\"\n\n" + + data).getBytes(); + byte[] bodyPartBigBytes = data.getBytes(); + byte[] intermediaryBytes = ("\n--" + BOUNDARY + + "\nContent-Disposition: form-data; name=\"msg2\"; filename=\"file2.txt\"\n\n" + + data).getBytes(); + byte[] finalBigBytes = ("\n" + "--" + BOUNDARY + "--\n").getBytes(); + ByteBuf firstBuf = Unpooled.wrappedBuffer(bodyStartBytes); + ByteBuf finalBuf = Unpooled.wrappedBuffer(finalBigBytes); + ByteBuf nextBuf; + if (big) { + nextBuf = Unpooled.wrappedBuffer(bodyPartBigBytes); + } else { + nextBuf = Unpooled.wrappedBuffer(intermediaryBytes); + } + DefaultHttpRequest req = + new DefaultHttpRequest(HttpVersion.HTTP_1_0, HttpMethod.POST, "/up"); + req.headers().add(HttpHeaderNames.CONTENT_TYPE, + "multipart/form-data; boundary=" + BOUNDARY); + + long start = System.nanoTime(); + + DefaultHttpDataFactory defaultHttpDataFactory = + new DefaultHttpDataFactory(noDisk? 1024 * 1024 : 16 * 1024); + HttpPostRequestDecoder decoder = + new HttpPostRequestDecoder(defaultHttpDataFactory, req); + firstBuf.retain(); + decoder.offer(new DefaultHttpContent(firstBuf)); + firstBuf.release(); + for (int i = 1; i < chunkNumber; i++) { + nextBuf.retain(); + decoder.offer(new DefaultHttpContent(nextBuf)); + nextBuf.release(); + nextBuf.readerIndex(0); + } + finalBuf.retain(); + decoder.offer(new DefaultLastHttpContent(finalBuf)); + finalBuf.release(); + while (decoder.hasNext()) { + InterfaceHttpData httpData = decoder.next(); + } + while (finalBuf.refCnt() > 0) { + finalBuf.release(); + } + while (nextBuf.refCnt() > 0) { + nextBuf.release(); + } + while (finalBuf.refCnt() > 0) { + finalBuf.release(); + } + long stop = System.nanoTime(); + double time = (stop - start) / 1000000.0; + defaultHttpDataFactory.cleanAllHttpData(); + defaultHttpDataFactory.cleanRequestHttpData(req); + decoder.destroy(); + return time; + } + + @Benchmark + public double multipartRequestDecoderHighDisabledLevel() { + final Level level = ResourceLeakDetector.getLevel(); + try { + ResourceLeakDetector.setLevel(Level.DISABLED); + return testHighNumberChunks(false, true); + } finally { + ResourceLeakDetector.setLevel(level); + } + } + + @Benchmark + public double multipartRequestDecoderBigDisabledLevel() { + final Level level = ResourceLeakDetector.getLevel(); + try { + ResourceLeakDetector.setLevel(Level.DISABLED); + return testHighNumberChunks(true, true); + } finally { + ResourceLeakDetector.setLevel(level); + } + } + + @Benchmark + public double multipartRequestDecoderHighSimpleLevel() { + final Level level = ResourceLeakDetector.getLevel(); + try { + ResourceLeakDetector.setLevel(Level.SIMPLE); + return testHighNumberChunks(false, true); + } finally { + ResourceLeakDetector.setLevel(level); + } + } + + @Benchmark + public double multipartRequestDecoderBigSimpleLevel() { + final Level level = ResourceLeakDetector.getLevel(); + try { + ResourceLeakDetector.setLevel(Level.SIMPLE); + return testHighNumberChunks(true, true); + } finally { + ResourceLeakDetector.setLevel(level); + } + } + + @Benchmark + public double multipartRequestDecoderHighAdvancedLevel() { + final Level level = ResourceLeakDetector.getLevel(); + try { + ResourceLeakDetector.setLevel(Level.ADVANCED); + return testHighNumberChunks(false, true); + } finally { + ResourceLeakDetector.setLevel(level); + } + } + + @Benchmark + public double multipartRequestDecoderBigAdvancedLevel() { + final Level level = ResourceLeakDetector.getLevel(); + try { + ResourceLeakDetector.setLevel(Level.ADVANCED); + return testHighNumberChunks(true, true); + } finally { + ResourceLeakDetector.setLevel(level); + } + } + + @Benchmark + public double multipartRequestDecoderHighParanoidLevel() { + final Level level = ResourceLeakDetector.getLevel(); + try { + ResourceLeakDetector.setLevel(Level.PARANOID); + return testHighNumberChunks(false, true); + } finally { + ResourceLeakDetector.setLevel(level); + } + } + + @Benchmark + public double multipartRequestDecoderBigParanoidLevel() { + final Level level = ResourceLeakDetector.getLevel(); + try { + ResourceLeakDetector.setLevel(Level.PARANOID); + return testHighNumberChunks(true, true); + } finally { + ResourceLeakDetector.setLevel(level); + } + } + +} diff --git a/microbench/src/main/java/io/netty/handler/codec/http/multipart/package-info.java b/microbench/src/main/java/io/netty/handler/codec/http/multipart/package-info.java new file mode 100644 index 0000000000..1f4cc119dc --- /dev/null +++ b/microbench/src/main/java/io/netty/handler/codec/http/multipart/package-info.java @@ -0,0 +1,19 @@ +/* + * Copyright 2020 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +/** + * Benchmarks for {@link io.netty.handler.codec.http.multipart}. + */ +package io.netty.handler.codec.http.multipart;