feat(#82): Subtract - additional method rather than issue#134
Open
leecampbell-codeagent wants to merge 7 commits intoHdrHistogram:mainfrom
Open
Conversation
71b3f39 to
89e3a13
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #82: Subtract — additional method rather than issue
Summary
The request is to add a
Subtractmethod toHistogramBasethat mirrors the existingAddmethod.The
Addmethod merges counts from one histogram into another by summing bucket counts.Subtractwould do the inverse: remove counts from one histogram by subtracting the source histogram's bucket counts from the target's.This is a natural complement to
Addand follows the same structural pattern (fast path for compatible structures, slow path for incompatible ones).Files Affected
HdrHistogram/HistogramBase.csSubtract(HistogramBase)virtual method (primary change); fix malformed exception message inAdd(missing))HdrHistogram.UnitTests/HistogramTestBase.csSubtractspec/tech-standards/api-reference.mdSubtractin the public API section alongsideAddConcrete implementations (
LongHistogram,IntHistogram,ShortHistogram,LongConcurrentHistogram,IntConcurrentHistogram) do not require changes because they inherit the base implementation and already implementAddToCountAtIndexwhich accepts negative values for subtraction.Acceptance Criteria
Subtract(HistogramBase fromHistogram)method exists onHistogramBasewith the same visibility asAdd(public virtual).TotalCount == 0.HighestTrackableValueexceeds the target's throwsArgumentOutOfRangeException(same guard asAdd).BucketCount,SubBucketCount,_unitMagnitude) uses the fast path (direct index iteration).ValueFromIndex).TotalCountis kept consistent after subtraction.Adddoc comment.Test Strategy
Add the following test methods to
HdrHistogram.UnitTests/HistogramTestBase.cs, directly below the existingAddtests:Subtract_should_reduce_the_counts_from_two_histogramsTestValueLevelandTestValueLevel * 1000intohistogramtwice each.TestValueLevelandTestValueLevel * 1000intootheronce each (same config).histogram.Subtract(other).histogram.GetCountAtValue(TestValueLevel) == 1L,histogram.GetCountAtValue(TestValueLevel * 1000) == 1L, andhistogram.TotalCount == 2L.Subtract_should_allow_small_range_histograms_to_be_subtractedbiggerOtherwith double the range; record same values into both.biggerOther.TotalCountdecrease correctly.Subtract_throws_if_other_has_a_larger_rangeArgumentOutOfRangeExceptionis thrown (mirrorsAdd_throws_if_other_has_a_larger_range).These three tests run against all concrete histogram types via the existing abstract base-test pattern (xUnit inheritance through
LongHistogramTests,IntHistogramTests,ShortHistogramTests, etc.).Implementation Notes
Validation
Copy the guard from
Addverbatim, applying the same fix to the malformed exception message (missing)) that will also be corrected inAddin the same PR:The existing
Addmessage at line 283 is missing the closing)after the first interpolated value.Fix it in
Addand use the corrected form inSubtract.Fast path
AddToCountAtIndexalready accepts negativeaddendvalues; passing the negated count is sufficient.Slow path
This exactly mirrors the
Addslow path (lines 299–303) with the sign flipped.RecordValueWithCountdoes not guard against negative counts — it callsAddToCountAtIndexdirectly with no validation — so no helper method is needed.Risks and Open Questions
Negative counts: The current design does not prevent bucket counts from going negative if more is subtracted than was recorded.
The Java reference implementation does not guard against this either; the initial implementation should follow the same lenient approach.
A future issue can address validation if needed.
Concurrent implementations:
LongConcurrentHistogramandIntConcurrentHistogramoverrideAddToCountAtIndexwith atomic operations.Because
Subtractin the base class usesAddToCountAtIndex(passing negated counts), concurrent safety should be inherited automatically — but this must be verified.TotalCountconsistency:AddToCountAtIndexincrementsTotalCountby the addend.Passing a negative addend should decrement
TotalCountcorrectly, keeping it consistent.Verify with
LongHistogrambefore relying on this for all types.Task breakdown
Task List: Issue #82 — Add
SubtractMethod toHistogramBaseImplementation
Fix malformed exception message in
AddinHistogramBase.csHdrHistogram/HistogramBase.cs, line 283)after the first interpolated value so the message reads"The other histogram covers a wider range ({fromHistogram.HighestTrackableValue}) than this one ({HighestTrackableValue})."the brief requires the same corrected form to be used in
Subtract.Add
Subtract(HistogramBase fromHistogram)method toHistogramBaseHdrHistogram/HistogramBase.cs, immediately after the closing brace ofAdd(currently around line 305)
public virtual void Subtract(HistogramBase fromHistogram)ArgumentOutOfRangeException(nameof(fromHistogram), ...)whenHighestTrackableValue < fromHistogram.HighestTrackableValue(use the fixed messageform from the task above).
BucketCount,SubBucketCount, and_unitMagnitudeall match):iterate
fromHistogram.CountsArrayLengthindices and callAddToCountAtIndex(i, -fromHistogram.GetCountAtIndex(i)).fromHistogram.CountsArrayLengthindices and callRecordValueWithCount(fromHistogram.ValueFromIndex(i), -fromHistogram.GetCountAtIndex(i)).AddToCountAtIndexaccepts negative addends; negating the source count is thecorrect inversion of
Addwithout any additional helper.HistogramBaseinstance.Add XML doc comment to
SubtractHdrHistogram/HistogramBase.cs, directly above the newSubtractmethod<summary>,<param name="fromHistogram">,<exception cref="System.ArgumentOutOfRangeException">Adddoc comment (lines 274–278).Add's XML doc block.Tests
Add
Subtract_should_reduce_the_counts_from_two_histogramstoHistogramTestBase.csHdrHistogram.UnitTests/HistogramTestBase.cs, after the lastAdd-related test(currently around line 260)
histogramwithDefaultHighestTrackableValue/DefaultSignificantFigures.TestValueLeveltwice andTestValueLevel * 1000twice intohistogram.otherwith same config; recordTestValueLevelonce andTestValueLevel * 1000once.histogram.Subtract(other).histogram.GetCountAtValue(TestValueLevel) == 1L,histogram.GetCountAtValue(TestValueLevel * 1000) == 1L,histogram.TotalCount == 2L.amount" and "TotalCount is kept consistent after subtraction" and "subtracting from
itself results in zero" (via symmetry check of counts).
[Fact], compiles, runs green for all concrete histogram types.Add
Subtract_should_allow_small_range_histograms_to_be_subtractedtoHistogramTestBase.csHdrHistogram.UnitTests/HistogramTestBase.cs, after the test abovebiggerOtherwithDefaultHighestTrackableValue * 2/DefaultSignificantFigures.histogramwithDefaultHighestTrackableValue/DefaultSignificantFigures.TestValueLevelandTestValueLevel * 1000once each into both histograms.biggerOther.Subtract(histogram)(subtract the smaller from the bigger).biggerOther.GetCountAtValue(TestValueLevel) == 0L,biggerOther.GetCountAtValue(TestValueLevel * 1000) == 0L,biggerOther.TotalCount == 0L.path" (exercises the slow path when
DefaultHighestTrackableValue * 2differs in bucketlayout from
DefaultHighestTrackableValue).[Fact], compiles, runs green for all concrete histogram types.Add
Subtract_throws_if_other_has_a_larger_rangetoHistogramTestBase.csHdrHistogram.UnitTests/HistogramTestBase.cs, after the test abovehistogramwithDefaultHighestTrackableValue/DefaultSignificantFigures.biggerOtherwithDefaultHighestTrackableValue * 2/DefaultSignificantFigures.ArgumentOutOfRangeExceptionis thrown when callinghistogram.Subtract(biggerOther).HighestTrackableValueexceeds the target's throws
ArgumentOutOfRangeException".[Fact], compiles, runs green for all concrete histogram types.Documentation
Subtractinspec/tech-standards/api-reference.mdspec/tech-standards/api-reference.md, lines 137–145 ("Histogram Operations"code block)
void Subtract(HistogramBase other) // Remove histogram valueson theline immediately after
void Add(HistogramBase other) // Merge histograms.Addis already listedthere and
Subtractis a direct complement.AddandSubtractadjacent to each other.Acceptance Criteria Cross-Reference
Subtractexists aspublic virtualonHistogramBaseSubtractmethodTotalCount == 0Subtract_should_reduce_the_counts_from_two_histograms(recording equal amounts)Subtract_should_reduce_the_counts_from_two_histogramsArgumentOutOfRangeExceptionwhen source range exceeds targetSubtract_throws_if_other_has_a_larger_rangeSubtractmethod (fast-path branch condition mirrorsAdd)Subtract_should_allow_small_range_histograms_to_be_subtracted(exercises slow path)TotalCountkept consistent after subtractionSubtract_should_reduce_the_counts_from_two_histograms(assertsTotalCount == 2L)Adddoc commentSubtractCloses #82