Bug 56953 - A improvement for "DataInputStream"
A improvement for "DataInputStream"
Status: RESOLVED WONTFIX
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
trunk
PC All
: P2 enhancement (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks: 56955
  Show dependency tree
 
Reported: 2014-09-11 05:51 UTC by hzhang9
Modified: 2014-11-25 15:47 UTC (History)
0 users



Attachments
patch (85.98 KB, patch)
2014-09-11 05:51 UTC, hzhang9
Details | Diff
test case (4.69 KB, text/plain)
2014-09-11 05:52 UTC, hzhang9
Details
patch (106.69 KB, patch)
2014-09-12 05:05 UTC, hzhang9
Details | Diff
change DataInputStream to DataInput (7.23 KB, patch)
2014-09-17 08:53 UTC, hzhang9
Details | Diff
FastDataInputStream implementation (5.88 KB, text/plain)
2014-09-17 08:56 UTC, hzhang9
Details
Test case (3.97 KB, text/plain)
2014-09-17 08:57 UTC, hzhang9
Details

Note You need to log in before you can comment on or make changes to this bug.
Description hzhang9 2014-09-11 05:51:38 UTC
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.
Comment 1 hzhang9 2014-09-11 05:52:11 UTC
Created attachment 31994 [details]
test case
Comment 2 Mark Thomas 2014-09-11 20:09:16 UTC
The patch is no good. It includes a whole bunch of changes unrelated to this issue.
Comment 3 hzhang9 2014-09-12 05:05:24 UTC
Created attachment 32008 [details]
patch
Comment 4 hzhang9 2014-09-12 05:39:58 UTC
(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?
Comment 5 Mark Thomas 2014-09-12 08:06:00 UTC
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.
Comment 6 Konstantin Kolinko 2014-09-12 10:48:12 UTC
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.
Comment 7 Mark Thomas 2014-09-12 13:28:26 UTC
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.
Comment 8 hzhang9 2014-09-15 08:08:52 UTC
OK, and I'm working on this.
Comment 9 Konstantin Kolinko 2014-09-15 09:17:41 UTC
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.
Comment 10 hzhang9 2014-09-17 08:53:03 UTC
Created attachment 32029 [details]
change DataInputStream to DataInput
Comment 11 hzhang9 2014-09-17 08:56:33 UTC
Created attachment 32030 [details]
FastDataInputStream implementation
Comment 12 hzhang9 2014-09-17 08:57:22 UTC
Created attachment 32031 [details]
Test case
Comment 13 hzhang9 2014-09-17 09:07:06 UTC
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.
Comment 14 Mark Thomas 2014-09-17 10:35:42 UTC
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.
Comment 15 Konstantin Kolinko 2014-09-17 10:47:42 UTC
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.
Comment 16 Mark Thomas 2014-09-17 12:49:53 UTC
Patch has been applied to 7.0.x for 7.0.56 onwards.
Comment 17 Mark Thomas 2014-09-17 13:01:29 UTC
Review comments applied.
Comment 18 hzhang9 2014-09-18 00:44:27 UTC
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.
Comment 19 Konstantin Kolinko 2014-11-08 02:12:58 UTC
Note that there was a regression caused by this change - bug 57173.
Comment 20 hzhang9 2014-11-10 00:54:49 UTC
(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.
Comment 21 Mark Thomas 2014-11-25 15:47:50 UTC
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.