[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-776112582 > Hi @XenoAmess > Please see git master and rebase. I updated with your changes for InputStream and Reader but not for the EOL APIs. If you still want those in, please rebase and follow the pattern I established with the *Benchmark classes. > TY! -) > PS: I gave you credit in the commit message and changes.xml 👍 rebased. benchmark redone. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-758804757 @garydgregory unsynced buffered classes deleted. re-run performance tests, results at https://pastebin.ubuntu.com/p/ZpZFHRMQkh/ please do a re-review. Thanks. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-755419401 > Hello @XenoAmess > We already have a lot of changes for the next release, so I want to manage expectations such that I would I prefer to get out 3.12 before making even more big changes like these. > > But still, let's continue this thread. Starting with the lowest-level bits: we need to justify the addition of the misnamed `Unsync*` classes, the prefix should be `Unsynchronized` like our existing `UnsynchronizedByteArrayInputStream`, which I've already mentioned. Perhaps our addition of UnsynchronizedByteArrayInputStream was a mistake since the Java folks make it sounds like these are superfluous in https://bugs.openjdk.java.net/browse/JDK-4097272, so I think we need to see a performance test that shows there is a clear performance benefit to adding those as valuable on their own. > > We need performances test that show the differences, if any, between the JRE's classes and our proposed `Unsynchronized` versions. Since you propose two such classes `UnsyncBufferedInputStream` and `UnsyncBufferedReader`, that's two new tests. Or did I miss these here? > > I think you should create a new PR for just these two new classes and their tests. This will make the work simpler for everyone when reviewing and testing. > > TY. @garydgregory Hi. I done the performance test at https://github.com/apache/commons-io/pull/184. The two classes is faster, but not as that faster as we thought. Detailed performance tests results: https://pastebin.ubuntu.com/p/TtDxYw4WVG/ This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-753646208 > > @garydgregory > > got it. > > new added class renamed as: > > org.apache.commons.io.input.buffer.UnsyncBufferedInputStream > > org.apache.commons.io.input.buffer.UnsyncBufferedReader > > org.apache.commons.io.input.buffer.LineEndUnifiedBufferedReader > > The Java folks make it sounds like these are superfluous in https://bugs.openjdk.java.net/browse/JDK-4097272, so I think we need to see a performance test that shows there is a clear performance benefit to adding those as valuable on their own. > > Perhaps our addition of `UnsynchronizedByteArrayInputStream` was a mistake. @garydgregory Hi. I already listed some performance test results 3 months ago, in this thread. please look back and see them. isn't the performance test in the list enough? If so , what other types of performance test do you need? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-749616807 > @garydgregory Weekend now. Any news？:) ping? :) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-748430204 @garydgregory Weekend now. Any news？:) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-745358426 > I think would like to/will soon-ish bring in the underlying `Unsynchronized*` classes first since they seem like useful general building blocks. Then this PR can be rebased. I would like to do this work at some point this week or at the weekend. Stay tuned. @garydgregory got it. Thanks. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-678616521 Hi. Is there anything that I should do/fix for this pr? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-669180712 > Do you have any proof that using UnsyncBufferedReader etc in contentEquals will result in a slow-down unless further changes are made to contentEquals? Yes, I ran a rough performance test on a same pc, and the result be time 9e8.(the current version of this pr shows 4.6e8, and master 2.8e9) > If so, what further changes are needed to contentEquals, and what is the speed improvement? That trick only be possible when we comparing two(or several) InputReaders. I shared an index on them, thus the two buffer's index be a same local variable. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-669144943 > Also, the existing prefix we use is Undynchronized, not Unsync. Gary > Undynchronized should presumably be Unsynchronized? > > However, I think that makes the class names rather long. > I think Unsync is clear enough as a prefix (and avoids the issue of whether to use -ized or -ised) I'm neutral on the naming question. If you get an agreement about it, I'm glad to rename the two classes according to the result. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-669143244 > AFAICT the new classes UnsyncBufferedReader etc are not actually used (except in test code). yes, they are not quite used, but used for being parent class of > I would expect all the contentEquals methods to remain exactly the same except for changes to the input sources. > The logic should remain unchanged (otherwise the new classes are not proper replacements). It can, but have huge performance lost. I also used another trick in those contentEquals functions, which cannot be used if we use that classes. And if we use the two classes there, will be 1 time slower than the functions we used in this pr (still be about 2 times faster than original) > Likewise I would not expect to see any changes to test cases, only new ones for the new classes, and perhaps a few new samples for existing methods where the coverage is incomplete. The changes in test are already moved to another pr, at https://github.com/apache/commons-io/pull/137 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-668359292 @garydgregory got it. new added class renamed as: org.apache.commons.io.input.buffer.UnsyncBufferedInputStream org.apache.commons.io.input.buffer.UnsyncBufferedReader org.apache.commons.io.input.buffer.LineEndUnifiedBufferedReader This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-665831472 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-665609575 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-640937650 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-639558681 @garydgregory Hi gary. please find some time for this pr. If this pr make sence, then I'll renew other `contentEquals` functions in the same class. thx. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118:
URL: https://github.com/apache/commons-io/pull/118#issuecomment-637036540
result json:
```
[
{
"jmhVersion" : "1.21",
"benchmark" :
"org.apache.commons.io.performance.IOUtilsContentEqualsPerformanceTest.testContentEqualsForFileNew",
"mode" : "avgt",
"threads" : 1,
"forks" : 5,
"jvm" : "C:\\jdk-13.0.2+8\\bin\\java.exe",
"jvmArgs" : [
],
"jdkVersion" : "13.0.2",
"vmName" : "OpenJDK 64-Bit Server VM",
"vmVersion" : "13.0.2+8",
"warmupIterations" : 5,
"warmupTime" : "10 s",
"warmupBatchSize" : 1,
"measurementIterations" : 5,
"measurementTime" : "10 s",
"measurementBatchSize" : 1,
"primaryMetric" : {
"score" : 7934046.779660092,
"scoreError" : 644395.8679898068,
"scoreConfidence" : [
7289650.911670285,
8578442.6476499
],
"scorePercentiles" : {
"0.0" : 7241780.811006517,
"50.0" : 7517702.629601804,
"90.0" : 9152312.968421623,
"95.0" : 1.0373390719340876E7,
"99.0" : 1.0726891103965702E7,
"99.9" : 1.0726891103965702E7,
"99.99" : 1.0726891103965702E7,
"99.999" : 1.0726891103965702E7,
"99." : 1.0726891103965702E7,
"100.0" : 1.0726891103965702E7
},
"scoreUnit" : "ns/op",
"rawData" : [
[
7459707.30797912,
8792593.848857645,
1.0726891103965702E7,
8550254.786324786,
8001410.311750599
],
[
7347818.649045521,
7251497.463768116,
7366967.231222386,
7420473.516320474,
7517702.629601804
],
[
7387172.525849335,
7278674.4,
7307894.88677867,
7363340.103016924,
7241780.811006517
],
[
7868122.955974842,
8014526.602564103,
7610742.99847793,
7339908.804108584,
7466605.9701492535
],
[
7641931.603053435,
8134969.268292683,
8823474.603174603,
9548556.488549618,
150.621669628
]
]
},
"secondaryMetrics" : {
}
},
{
"jmhVersion" : "1.21",
"benchmark" :
"org.apache.commons.io.performance.IOUtilsContentEqualsPerformanceTest.testContentEqualsNew2",
"mode" : "avgt",
"threads" : 1,
"forks" : 5,
"jvm" : "C:\\jdk-13.0.2+8\\bin\\java.exe",
"jvmArgs" : [
],
"jdkVersion" : "13.0.2",
"vmName" : "OpenJDK 64-Bit Server VM",
"vmVersion" : "13.0.2+8",
"warmupIterations" : 5,
"warmupTime" : "10 s",
"warmupBatchSize" : 1,
"measurementIterations" : 5,
"measurementTime" : "10 s",
"measurementBatchSize" : 1,
"primaryMetric" : {
"score" : 3.4456759445822275E8,
"scoreError" : 2.110473359059532E7,
"scoreConfidence" : [
3.2346286086762744E8,
3.6567232804881805E8
],
"scorePercentiles" : {
"0.0" : 3.183939625E8,
"50.0" : 3.3247121612903225E8,
"90.0" : 4.0086423015384614E8,
"95.0" : 4.1518499175E8,
"99.0" : 4.172705625E8,
"99.9" : 4.172705625E8,
"99.99" : 4.172705625E8,
"99.999" : 4.172705625E8,
"99." : 4.172705625E8,
"100.0" : 4.172705625E8
},
"scoreUnit" : "ns/op",
"rawData" : [
[
3.725598037037037E8,
3.7222124E8,
3.945612769230769E8,
4.1031866E8,
4.172705625E8
],
[
3.3457967E8,
3.24313664516129E8,
3.4986420344827586E8,
3.450001724137931E8,
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-637035997 @garydgregory Performance test done. **Conclution:** In cases for small content readers, the new function is slightly faster. but in cases for large content, the new function can be 6.5 times faster. ``` D:\workspace\commons-io>mvn -Pbenchmark [INFO] Scanning for projects... [INFO] [INFO] ---< commons-io:commons-io > [INFO] Building Apache Commons IO 2.7.1-SNAPSHOT [INFO] [ jar ]- [INFO] [INFO] --- maven-clean-plugin:3.1.0:clean (default-clean) @ commons-io --- [INFO] Deleting D:\workspace\commons-io\target [INFO] [INFO] --- maven-enforcer-plugin:3.0.0-M2:enforce (enforce-maven-version) @ commons-io --- [INFO] [INFO] --- maven-enforcer-plugin:3.0.0-M2:enforce (enforce-maven-3) @ commons-io --- [INFO] [INFO] --- apache-rat-plugin:0.13:check (rat-check) @ commons-io --- [INFO] Enabled default license matchers. [INFO] Will parse SCM ignores for exclusions... [INFO] Parsing exclusions from D:\workspace\commons-io\.gitignore [INFO] Finished adding exclusions from SCM ignore files. [INFO] 73 implicit excludes (use -debug for more details). [INFO] 12 explicit excludes (use -debug for more details). [INFO] 373 resources included (use -debug for more details) [INFO] Rat check: Summary over all files. Unapproved: 0, unknown: 0, generated: 0, approved: 364 licenses. [INFO] [INFO] --- build-helper-maven-plugin:3.0.0:parse-version (parse-version) @ commons-io --- [INFO] [INFO] --- maven-antrun-plugin:1.8:run (javadoc.resources) @ commons-io --- [INFO] Executing tasks main: [copy] Copying 2 files to D:\workspace\commons-io\target\apidocs\META-INF [INFO] Executed tasks [INFO] [INFO] --- maven-remote-resources-plugin:1.5:process (process-resource-bundles) @ commons-io --- [INFO] [INFO] --- buildnumber-maven-plugin:1.4:create (default) @ commons-io --- [INFO] Executing: cmd.exe /X /C "git rev-parse --verify HEAD" [INFO] Working directory: D:\workspace\commons-io [INFO] Storing buildNumber: 73dc0ff3e6d79ab88f1ed0d253d52cb6da0dffa5 at timestamp: 1591034434847 [INFO] Storing buildScmBranch: refine_contentEquals [INFO] [INFO] --- maven-resources-plugin:3.1.0:resources (default-resources) @ commons-io --- [INFO] Using 'iso-8859-1' encoding to copy filtered resources. [INFO] skip non existing resourceDirectory D:\workspace\commons-io\src\main\resources [INFO] Copying 2 resources to META-INF [INFO] [INFO] --- maven-compiler-plugin:3.8.1:compile (default-compile) @ commons-io --- [INFO] Changes detected - recompiling the module! [INFO] Compiling 158 source files to D:\workspace\commons-io\target\classes [INFO] /D:/workspace/commons-io/src/main/java/org/apache/commons/io/input/ClassLoaderObjectInputStream.java: Some input files use or override a deprecated API. [INFO] /D:/workspace/commons-io/src/main/java/org/apache/commons/io/input/ClassLoaderObjectInputStream.java: Recompile with -Xlint:deprecation for details. [INFO] /D:/workspace/commons-io/src/main/java/org/apache/commons/io/IOExceptionList.java: D:\workspace\commons-io\src\main\java\org\apache\commons\io\IOException List.java uses unchecked or unsafe operations. [INFO] /D:/workspace/commons-io/src/main/java/org/apache/commons/io/IOExceptionList.java: Recompile with -Xlint:unchecked for details. [INFO] [INFO] --- maven-bundle-plugin:4.2.1:manifest (bundle-manifest) @ commons-io --- [INFO] [INFO] --- animal-sniffer-maven-plugin:1.18:check (checkAPIcompatibility) @ commons-io --- [INFO] Checking unresolved references to org.codehaus.mojo.signature:java18:1.0 [INFO] D:\workspace\commons-io\src\main\java\org\apache\commons\io\input\TeeReader.java:158: Covariant return type change detected: java.nio.Buffer java.nio.Char Buffer.position(int) has been changed to java.nio.CharBuffer java.nio.CharBuffer.position(int) [INFO] D:\workspace\commons-io\src\main\java\org\apache\commons\io\input\TeeReader.java:158: Covariant return type change detected: java.nio.Buffer java.nio.Char Buffer.limit(int) has been changed to java.nio.CharBuffer java.nio.CharBuffer.limit(int) [INFO] D:\workspace\commons-io\src\main\java\org\apache\commons\io\input\TeeReader.java:162: Covariant return type change detected: java.nio.Buffer java.nio.Char Buffer.position(int) has been changed to java.nio.CharBuffer java.nio.CharBuffer.position(int) [INFO] D:\workspace\commons-io\src\main\java\org\apache\commons\io\input\TeeReader.java:162: Covariant return type change detected: java.nio.Buffer java.nio.Char Buffer.limit(int) has been changed to java.nio.CharBuffer java.nio.CharBuffer.limit(int) [INFO] D:\workspace\commons-io\src\ma
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-637013520 @garydgregory BTW, how can I show the performance result generated? simply paste it here? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-636996577 @garydgregory I will use this as example and change its content and add the test into this repo if you don't mind. https://github.com/apache/commons-lang/blob/5cdac9cfd5a74b0a52ebde32798b973c6edbaa79/src/test/java/org/apache/commons/lang3/HashSetvBitSetTest.java This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-636994263 > @XenoAmess > > * When you create a PR, please do not leave the description empty. It makes it harder to review. OK > * If a PR claims a performance improvement, it must back it up; some components do this using JMH microbenchmarks, for example please see Apache Commons BCEL, Crypto, CSV, Lang, Math, RNG. I have only performance using Jprofiler and I don't know how to embed a test with performance in junit. I'll learn about JMH microbenchmarks. thanks. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-636974841 I also want to change other contentEquals functions in IOUtils (using similar way), but I want to listen to your advices first. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-io] XenoAmess commented on pull request #118: [IO-670] refine IOUtils.contentEquals(Reader, Reader)
XenoAmess commented on pull request #118: URL: https://github.com/apache/commons-io/pull/118#issuecomment-636971532 need I make a performance test? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
