Created attachment 31993 [details] patch The method "readUnsignedShort()" of "DataInputStream" read the stream twice to get the unsigned short. This happens even if the "BufferedInputStream" is invoked. public final int readUnsignedShort() throws IOException { int ch1 = in.read(); int ch2 = in.read(); if ((ch1 | ch2) < 0) throw new EOFException(); return (ch1 << 8) + (ch2 << 0); } It may cause extra cost for some boundary processing. This case also appears in "readInt", "readChar", etc. It is obvious in some large projects. Use a interface to replace "DataInputStream" by "FastDataInputStream" can bypass these processes. The "FastDataInputStream" gets the bytes from buffer directly. //========== method in FastDataInputStream. ========== public int readUnsignedShort() throws IOException{ if(pos + 1 >= cnt) { fill(); if(pos + 1 >= cnt) throw new EOFException(); } int ch1 = this.buf[pos++] & 0xff; int ch2 = this.buf[pos++] & 0xff; return (ch1 << 8) + (ch2 << 0); } Benefit shows bellow, it is got from the test case in attachment. =====lots of jar files===== DataInputStream: 592 FastDataInputStream: 488 =====few jar files===== DataInputStream: 93 FastDataInputStream: 77 notice: The optimized method is called before original method in the test case, so the real result should be more obvious.
Created attachment 31994 [details] test case
The patch is no good. It includes a whole bunch of changes unrelated to this issue.
Created attachment 32008 [details] patch
(In reply to Mark Thomas from comment #2) > The patch is no good. It includes a whole bunch of changes unrelated to this > issue. It is indeed a problem. The change should be simpler than it shows in the patch, we just need to design a new stream for the parse. But the methods in "DataInputStream" are always be "final", so I can only use a interface to implement it. Do you have any suggestion for this situation?
If you continue to ignore the comments you are given then this issue is going to get closed as WONTFIX. Your first patch was 86k and full of irrelevant changes. The second attempt is worse at 107k. Starting at the beginning of the patch: 1. The first chunk changes restores an svn keyword the Tomcat team previously removed. This change has nothing to do with this issue and should not be in this patch. 2. The second chunk reverts a fix to a constant name the Tomcat team previously fixed and removes some code necessary for Java 8 support. This change has nothing to do with this issue and should not be in this patch. Further this change breaks Java 8 support. 3. The third chunk makes further changes that have nothing to do with this issue and further breaks Java 8 support. And so on. I have no problem with reviewing a patch that completely replaces DataInputStream because of final methods. It is all the other irrelevant, breaking changes in the patch that are the problem.
The point of java.io.BufferedInputStream() is that is.read() were fast enough. I do not see much benefit in re-implementing standard JRE classes. By the way, is.read() is a blocking method. If you were to use fill() or is.read(chars[]) there is no guarantee of how many bytes they can actually read in a single call. That is why DataInput.readFully() method is there.
With the various other changes and improvements back-ported to 7.0.x, I'm prepared to consider this idea if the benefit justifies it. That said, we need a patch that implements this idea - and just this idea - first.
OK, and I'm working on this.
A large portion of "your" code is apparently borrowed from some software that uses GPL license. You cannot contribute such code to an Apache project.
Created attachment 32029 [details] change DataInputStream to DataInput
Created attachment 32030 [details] FastDataInputStream implementation
Created attachment 32031 [details] Test case
Sorry for the delay. New patch applies on TRUNK, and the codes borrowed from GPL has been changed. Benefit shows bellow(300 jar files involved in this test): =====Call FDIS first===== DataInputStream: 7342 FastDataInputStream: 6967 =====reverse call sequency===== DataInputStream: 7369 FastDataInputStream: 6979 The benefit is considerable when there are 200 or more jar files need to be parse.
Thanks for the updated patches. I see a 20-25% improvement with the patch so it has been applied to 8.0.x for 8.0.13 onwards. I'll look into porting it to 7.0.x.
OK, this is better. 1. Formatting: the code shall not use tab characters 2. In "skipBytes(int n)": there is no reason to call "fillNew()" after calling "in.skip(n - sum)" on the underlying stream. If another skip call follows then there is no point in filling the buffer. 3. "<< 0" shift operation is NOOP and can be removed. 4. I wonder whether "ch + ch" or "ch | ch" works better. In theory the latter should be faster, but I guess there is no measurable difference nowadays. 5. In uninmplemented readLine() method: maybe better throw new java.lang.UnsupportedOperationException() instead of IOException.
Patch has been applied to 7.0.x for 7.0.56 onwards.
Review comments applied.
Thanks for your advice. These points are extremely wise. (In reply to Konstantin Kolinko from comment #15) > OK, this is better. > > 1. Formatting: the code shall not use tab characters > > 2. In "skipBytes(int n)": there is no reason to call "fillNew()" after > calling "in.skip(n - sum)" on the underlying stream. If another skip call > follows then there is no point in filling the buffer. > > 3. "<< 0" shift operation is NOOP and can be removed. > > 4. I wonder whether "ch + ch" or "ch | ch" works better. In theory the > latter should be faster, but I guess there is no measurable difference > nowadays. > > 5. In uninmplemented readLine() method: maybe better throw new > java.lang.UnsupportedOperationException() instead of IOException.
Note that there was a regression caused by this change - bug 57173.
(In reply to Konstantin Kolinko from comment #19) > Note that there was a regression caused by this change - bug 57173. I checked the case, and the problem does exist because I didn't implement several method on it. I'll try to fix it later. The suggestion to roll back the change can be adopted temporarily.
This fix has been reverted from trunk, 8.0.x (for 8.0.16 onwards) and 7.0.x (for 7.0.58 onwards) and will not be reapplied. Once the issues causing the regressions found so far are fixed, the performance improvement is only a few percent. It simply isn't worth the risk of further regressions to shave a few percent of the scanning time at application start.