[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 Mark Thomas changed: What|Removed |Added Resolution|FIXED |WONTFIX --- Comment #21 from Mark Thomas --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #20 from hzha...@ebay.com --- (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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #19 from Konstantin Kolinko --- Note that there was a regression caused by this change - bug 57173. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #18 from hzha...@ebay.com --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 Mark Thomas changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #17 from Mark Thomas --- Review comments applied. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #16 from Mark Thomas --- Patch has been applied to 7.0.x for 7.0.56 onwards. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #15 from Konstantin Kolinko --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #14 from Mark Thomas --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #13 from hzha...@ebay.com --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #12 from hzha...@ebay.com --- Created attachment 32031 --> https://issues.apache.org/bugzilla/attachment.cgi?id=32031&action=edit Test case -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #11 from hzha...@ebay.com --- Created attachment 32030 --> https://issues.apache.org/bugzilla/attachment.cgi?id=32030&action=edit FastDataInputStream implementation -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 hzha...@ebay.com changed: What|Removed |Added Attachment #32008|0 |1 is obsolete|| --- Comment #10 from hzha...@ebay.com --- Created attachment 32029 --> https://issues.apache.org/bugzilla/attachment.cgi?id=32029&action=edit change DataInputStream to DataInput -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #9 from Konstantin Kolinko --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #8 from hzha...@ebay.com --- OK, and I'm working on this. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #7 from Mark Thomas --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #6 from Konstantin Kolinko --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #5 from Mark Thomas --- 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. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #4 from hzha...@ebay.com --- (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? -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 hzha...@ebay.com changed: What|Removed |Added Attachment #31993|0 |1 is obsolete|| Attachment #31994|0 |1 is obsolete|| --- Comment #3 from hzha...@ebay.com --- Created attachment 32008 --> https://issues.apache.org/bugzilla/attachment.cgi?id=32008&action=edit patch -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 Mark Thomas changed: What|Removed |Added OS||All --- Comment #2 from Mark Thomas --- The patch is no good. It includes a whole bunch of changes unrelated to this issue. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 hzha...@ebay.com changed: What|Removed |Added Blocks||56955 -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 hzha...@ebay.com changed: What|Removed |Added Attachment #31993|0 |1 is patch|| -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56953] A improvement for "DataInputStream"
https://issues.apache.org/bugzilla/show_bug.cgi?id=56953 --- Comment #1 from hzha...@ebay.com --- Created attachment 31994 --> https://issues.apache.org/bugzilla/attachment.cgi?id=31994&action=edit test case -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
