-
Notifications
You must be signed in to change notification settings - Fork 30
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.
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
A fix for HistogramBase.AllValues() returning a repeat of the last HistogramIterationValue #64
base: master
Are you sure you want to change the base?
Conversation
…e filled with the last HistogramIterationValue.
✅ Build HdrHistogram.NET 2.7.0-ibjxfkkf completed (commit 27c87e615f by @) |
1 similar comment
✅ Build HdrHistogram.NET 2.7.0-ibjxfkkf completed (commit 27c87e615f by @) |
Thanks for the PR @mtikilay |
@@ -2,14 +2,14 @@ | |||
|
|||
<PropertyGroup> | |||
<OutputType>Exe</OutputType> | |||
<TargetFrameworks>net47;netcoreapp1.1</TargetFrameworks> | |||
<TargetFrameworks>net471;netcoreapp1.1</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not targeting 4.7.1, but still 4.7. I think this would be a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also breaks the build.cmd file as it does hard code reference the .\HdrHistogram.Benchmarking\bin\Release\net47 path :-(
private HistogramIterationValue _current; | ||
public HistogramIterationValue Current | ||
{ | ||
get => _current.Clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the confusion here, but this is a low allocation path. Now that you are cloning (instead of using the value inplace) you are now allocating.
Consumers are welcome to execute a memberwise clone as they fetch the next value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually(!), having just run the benchmarks on master
and this PR, there is no significant performacne change (improvement or degradation), so the spirit of this PR is valid.
:-)
@@ -255,6 +255,12 @@ Next can you please ensure that your PR (Pull Request) has a comment in it descr | |||
Ideally if it is fixing an issue or a bug, there would be a Unit Test proving the fix and a reference to the Issues in the PR comments. | |||
|
|||
|
|||
### How to run tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding documentation like this.
However, doesn't the code from https://github.com/HdrHistogram/HdrHistogram.NET/blob/master/build.cmd#L15 not only already do this, but do it in the idiomatic dotnet cli style instead of calling out to hard coded paths with versions in them etc..
dotnet test .\HdrHistogram.UnitTests\HdrHistogram.UnitTests.csproj -v=q --no-build -c=Release
@@ -103,5 +103,7 @@ public override string ToString() | |||
", Percentile:" + Percentile + | |||
", PercentileLevelIteratedTo:" + PercentileLevelIteratedTo; | |||
} | |||
|
|||
public HistogramIterationValue Clone() => (HistogramIterationValue) this.MemberwiseClone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No XML comments on a public member/method
Thanks @mtikilay for your support. Help from the community makes Open Source work! 1 Mutable Enumerated ValuesI think that maybe keeping the port of the Java mutable value is not useful. You have proved that it is non-intuitive, and arguably(?) not idiomatic 2 Unit testsI am happy to make some changes if running unit tests isn't working in VS out of the box. However, it must keep the existing tests and benchmarks working. It is clear that the documentation/readme is lacking on how to run the unit tests and benchmarks. See new issue #66 3 Test and benchmark versionsThis is a library, so I want to keep the broadest scope available to my clients. I believe but could be wrong, that currently the best way to do this is to target ".NET 4.7" and "netcoreapp1.1". 4 Ideally one PR changes one thingMaybe I am just being a stick in the mud, but I kinda prefer that if we get a PR that is solves on thing, and solves it really well e.g. Mutable Enumerated Values #65. Also it is nice if you create the issue first, so it isn't too much of a surprise to the maintainer when the PR comes in. This give us both a channel to discuss the change. Keeping the PRs tight and specific also allows me to take bits of your work, without having reject all of it some of it isn't ideal. |
Hi @LeeCampbell -- thanks for your detailed reply. Apologies for taking a while to get back to you. First of all, yes, let's scrap this PR and I'll do this properly in two separate ones that address just the issues in question. I agree that one PR per one thing is definitely the right approach. So apologies for a sloppy PR and the mashing of framework version numbers and so on -- I was just trying to build the project on a vanilla VS2017 setup and it wasn't happy out of the box for some reason. I'll drop those bits from the actual commit, but it would definitely be good to have the project painlessly build and run on a standard install of the current Visual Studio. Regarding the how-to on running tests, your line is perfectly fine. I'm just not yet familiar with the standard ways of setting up and running tests projects in the .Net Core world. As for the actual reason I put the PR in, namely the issue of the mutating .Current and the odd result of AllValues().ToList(), I think that yes, the expectation of an average .Net dev (e.g. me!) would be that you wouldn't have to iterate and copy iterator.Current yourself. I do understand your concerns regarding allocation but this should probably be either clearly signposted with documentation and more descriptive method names, or behave like other IEnumerables people are used to... Perhaps try to keep both approaches: expose IEnumerator-returning methods as they are and just enforce cloning in those HistogramBase methods that return IEnumerables? I'll see if I can make that look nice. Also, using .MemberwiseClone does feel a bit dirty -- I'll change it to explicit object creation to make clear what is happening. I'll try to get this done in the next couple of weeks. Cheers, |
I found that HistogramBase.AllValues() was returning an IEnumerable that contained the same HistogramIterationValue, which was the value of the last bucket in the histogram. This PR contains a test and a fix for this problem.
Additionally, I also had some trouble building the project and running the tests on an out-of-the-box VS2017, so did some work to make that happen.