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

A fix for HistogramBase.AllValues() returning a repeat of the last HistogramIterationValue #64

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

A fix for HistogramBase.AllValues() returning a repeat of the last HistogramIterationValue #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34dea62
Updated README with a how-to-run-tests-if-they-don't-show-up-in-VS.
Apr 30, 2018
0ab1485
Upped library and framework versions to current ones.
Apr 30, 2018
6ec7562
Added test to expose the issue of AllValues() returning an IEnumerabl…
Apr 30, 2018
b236c0c
Added fix for bug exposed in 6ec756277162a0ffc924ca0566bfbbe695e60fa1.
Apr 30, 2018
558d412
Added some bits to .gitignore to ignore VS2017 working files.
Apr 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,8 @@ _UpgradeReport_Files/
Backup*/
UpgradeLog*.XML

Thumbs.db
Thumbs.db
.vs/HdrHistogram/v15/Server/sqlite3/storage.ide-wal
.vs/HdrHistogram/v15/Server/sqlite3/storage.ide-shm
.vs/HdrHistogram/v15/Server/sqlite3/storage.ide
.vs/HdrHistogram/v15/Server/sqlite3/db.lock
4 changes: 2 additions & 2 deletions HdrHistogram.Benchmarking/HdrHistogram.Benchmarking.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFrameworks>net47;netcoreapp1.1</TargetFrameworks>
<TargetFrameworks>net471;netcoreapp1.1</TargetFrameworks>
Copy link

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

Copy link

@vgw-LeeCampbell vgw-LeeCampbell May 8, 2018 •
edited
Loading

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 :-(

</PropertyGroup>

<ItemGroup>
<PackageReference Include="BenchmarkDotNet" Version="0.10.8" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'net47'">
<ItemGroup Condition="'$(TargetFramework)' == 'net471'">
<PackageReference Include="BenchmarkDotNet.Diagnostics.Windows">
<Version>0.10.8</Version>
</PackageReference>
Expand Down
2 changes: 1 addition & 1 deletion HdrHistogram.Examples/HdrHistogram.Examples.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp1.1</TargetFramework>
<TargetFramework>netcoreapp2.0</TargetFramework>
</PropertyGroup>

<ItemGroup>
Expand Down
5 changes: 3 additions & 2 deletions HdrHistogram.UnitTests/HdrHistogram.UnitTests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,10 @@

<ItemGroup>
<PackageReference Include="FluentAssertions" Version="4.19.2" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.0.0" />
<PackageReference Include="xunit" Version="2.2.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.7.0" />
<PackageReference Include="xunit" Version="2.3.1" />
<PackageReference Include="Xunit.Combinatorial" Version="1.2.1" />
<PackageReference Include="xunit.runner.console" Version="2.3.1" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.2.0" />
</ItemGroup>

Expand Down
18 changes: 18 additions & 0 deletions HdrHistogram.UnitTests/HistogramEncodingTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using HdrHistogram.Encoding;
using HdrHistogram.Utilities;
using Xunit;
using FluentAssertions;
using System.Linq;

namespace HdrHistogram.UnitTests
{
Expand All @@ -21,6 +23,22 @@ public void Given_a_populated_Histogram_When_encoded_and_decoded_Then_data_is_pr
HistogramAssert.AreValueEqual(source, result);
}

[Fact]
public void Given_a_populated_Histogram_iterating_over_buckets_gives_all_buckets()
{
var source = Create(DefaultHighestTrackableValue, DefaultSignificantFigures);
Load(source);
Iteration.HistogramIterationValue lastSeen = null;
foreach ( var v in source.AllValues().ToList() )
{
if ( lastSeen != null )
{
v.Should().NotBe( lastSeen );
}
lastSeen = v;
}
}

[Fact]
public void Given_a_populated_Histogram_When_encoded_and_decoded_with_compression_Then_data_is_preserved()
{
Expand Down
9 changes: 7 additions & 2 deletions HdrHistogram/Iteration/AbstractHistogramEnumerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ internal abstract class AbstractHistogramEnumerator : IEnumerator<HistogramItera
protected int CurrentSubBucketIndex { get; private set; }
protected long TotalCountToCurrentIndex { get; private set; }
protected long CountAtThisValue { get; private set; }

public HistogramIterationValue Current { get; private set; }

private HistogramIterationValue _current;
public HistogramIterationValue Current
{
get => _current.Clone();
Copy link

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.

Copy link

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.
:-)

private set => _current = value;
}

protected AbstractHistogramEnumerator(HistogramBase histogram)
{
Expand Down
2 changes: 2 additions & 0 deletions HdrHistogram/Iteration/HistogramIterationValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,5 +103,7 @@ public override string ToString()
", Percentile:" + Percentile +
", PercentileLevelIteratedTo:" + PercentileLevelIteratedTo;
}

public HistogramIterationValue Clone() => (HistogramIterationValue) this.MemberwiseClone();
Copy link

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

}
}
6 changes: 6 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link

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

If you are having trouble running xunit tests out of the box, it is possible to run them using a version of the command line below.
You'll just need to fill in the correct value of $(SolutionDir), which should be as defined in Visual Studio.
PS> dotnet C:\Users\%USERNAME%\.nuget\packages\xunit.runner.console\2.3.1\tools\netcoreapp2.0\xunit.console.dll $(SolutionDir)\HdrHistogram.UnitTests\bin\Debug\netcoreapp1.1\HdrHistogram.UnitTests.dll


HdrHistogram Details
----------------------------------------------

Expand Down