Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[IO-698] add Unsynchronized version of UnsynchronizedBufferedInputStream and UnsynchronizedBufferedInputStream, and performance tests. #184

Closed
wants to merge 1 commit into from
Closed

[IO-698] add Unsynchronized version of UnsynchronizedBufferedInputStream and UnsynchronizedBufferedInputStream, and performance tests. #184

wants to merge 1 commit into from

Conversation

XenoAmess
Copy link
Contributor

No description provided.

@XenoAmess XenoAmess mentioned this pull request Jan 6, 2021
Open
@XenoAmess
Copy link
Contributor Author

Detailed performance tests results: https://pastebin.ubuntu.com/p/TtDxYw4WVG/

@garydgregory
Copy link
Member

garydgregory commented Jan 6, 2021 •
edited
Loading

Something is not quite right with this set up because the output starts with:

Building Apache Commons IO 2.7.1-SNAPSHOT

instead of 2.9.0-SNAPSHOT.

Am I reading these results right? IOUtilsContentEqualsPerformanceTest.testBufferedInputStream2 is 0.001% faster than IOUtilsContentEqualsPerformanceTest.testBufferedInputStream which is within the margin of error? If so, it's not worth it especially since the highest "2" score is higher than the highest normal score (using the error margin).

garydgregory
garydgregory reviewed Jan 6, 2021
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
garydgregory
garydgregory reviewed Jan 6, 2021
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
@XenoAmess XenoAmess force-pushed the add_unsynchronized_input_stream branch from bd9fd8a to 7b61fb2 Compare January 10, 2021 15:45
@coveralls
Copy link

coveralls commented Jan 10, 2021 •
edited
Loading

Coverage Status

Coverage decreased (-0.001%) to 88.742% when pulling 36749e7 on xenoamess-fork:add_unsynchronized_input_stream into 4b92e65 on apache:master.

@XenoAmess XenoAmess force-pushed the add_unsynchronized_input_stream branch from 7b61fb2 to 0a96e61 Compare January 10, 2021 16:12
@XenoAmess
Copy link
Contributor Author

Something is not quite right with this set up because the output starts with:

Building Apache Commons IO 2.7.1-SNAPSHOT

instead of 2.9.0-SNAPSHOT.

because the original pr is done when 2.7.0-SNAPSHOT, and delayed to now :)
Alright, I've already rebased it, so it is with latest master now.

@XenoAmess
add Unsynchronized version of UnsynchronizedBufferedInputStream and U…
36749e7 
…nsynchronizedBufferedInputStream, and performance tests.

exec-maven-plugin.version to 3.0.0
Don't initialize values to their defaults.
It would simpler to validate this test if it was factored in such a way that a worker method was given a stream such that its called can call it with a constructed object, once for the JRE stream, another with the custom stream.A "2" postfix for a method name says nothing about why its different from the plain method name. Please give it a name that better describes what you are testing.
@XenoAmess XenoAmess force-pushed the add_unsynchronized_input_stream branch from 3fd2774 to 36749e7 Compare January 10, 2021 17:07
@XenoAmess
Copy link
Contributor Author

XenoAmess commented Jan 10, 2021 •
edited
Loading

@garydgregory rerun performance tests:
https://pastebin.ubuntu.com/p/njnG5x78fD/

seems no significant performance improvement in this two classes (comparing with sync ones in original jdk).
so maybe I should close this pr and migrate [IO-670] to use the version in jdk.
What is your opinion?

@garydgregory
Copy link
Member

@garydgregory rerun performance tests:
https://pastebin.ubuntu.com/p/njnG5x78fD/

seems no significant performance improvement in this two classes (comparing with sync ones in original jdk).
so maybe I should close this pr and migrate [IO-670] to use the version in jdk.
What is your opinion?

@arturobernalg
Am I reading the summary correctly in that the average times of some tests for the new code is slight slower?

@XenoAmess
Copy link
Contributor Author

@garydgregory rerun performance tests:
https://pastebin.ubuntu.com/p/njnG5x78fD/
seems no significant performance improvement in this two classes (comparing with sync ones in original jdk).
so maybe I should close this pr and migrate [IO-670] to use the version in jdk.
What is your opinion?

@arturobernalg
Am I reading the summary correctly in that the average times of some tests for the new code is slight slower?

@garydgregory seems so.
wondering why, but have no very good ideas.

@garydgregory
Copy link
Member

@garydgregory rerun performance tests:
https://pastebin.ubuntu.com/p/njnG5x78fD/
seems no significant performance improvement in this two classes (comparing with sync ones in original jdk).
so maybe I should close this pr and migrate [IO-670] to use the version in jdk.
What is your opinion?

@arturobernalg
Am I reading the summary correctly in that the average times of some tests for the new code is slight slower?

@garydgregory seems so.
wondering why, but have no very good ideas.

@arturobernalg
So... if it's only a tiny bit better on average (<0.01%) but can be worse in some cases, and also worse if you take into account the error measurement, then...?

@XenoAmess
Copy link
Contributor Author

@garydgregory rerun performance tests:
https://pastebin.ubuntu.com/p/njnG5x78fD/
seems no significant performance improvement in this two classes (comparing with sync ones in original jdk).
so maybe I should close this pr and migrate [IO-670] to use the version in jdk.
What is your opinion?

@arturobernalg
Am I reading the summary correctly in that the average times of some tests for the new code is slight slower?

@garydgregory seems so.
wondering why, but have no very good ideas.

@arturobernalg
So... if it's only a tiny bit better on average (<0.01%) but can be worse in some cases, and also worse if you take into account the error measurement, then...?

@garydgregory Yep, so maybe I should close this pr and migrate [IO-670] to use the version in jdk.

@XenoAmess XenoAmess closed this Jan 12, 2021
garydgregory added a commit that referenced this pull request Apr 27, 2023
@garydgregory
Bump commons-parent from 56 to 57 (#454) #184
35dfd15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garydgregory garydgregory Awaiting requested review from garydgregory

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@XenoAmess @garydgregory @coveralls