fix bugs on add & subtract

Author: alexvictoor

assembly/Histogram.ts

@@ -672,7 +672,10 @@ export default class Histogram<T, U> extends AbstractHistogramBase<T, U> {
         otherHistogram.maxValue
       );
       let otherCount = otherHistogram.getCountAtIndex(otherMaxIndex);
-      this.recordCountAtValue(otherCount, otherHistogram.maxValue);
+      this.recordCountAtValue(
+        otherCount,
+        otherHistogram.valueFromIndex(otherMaxIndex)
+      );
 
       // Record the remaining values, up to but not including the max value:
       for (let i = 0; i < otherMaxIndex; i++) {
@@ -756,12 +759,15 @@ export default class Histogram<T, U> extends AbstractHistogramBase<T, U> {
     ) {
       // optim
       // Counts arrays are of the same length and meaning, so we can just iterate and add directly:
+      let observedOtherTotalCount: u64 = 0;
       for (let i = 0; i < otherHistogram.countsArrayLength; i++) {
         const otherCount = otherHistogram.getCountAtIndex(i);
         if (otherCount > 0) {
           this.addToCountAtIndex(i, -otherCount);
+          observedOtherTotalCount += otherCount;
         }
       }
+      this.totalCount = this.totalCount - observedOtherTotalCount;
     } else {
       for (let i = 0; i < otherHistogram.countsArrayLength; i++) {
         const otherCount = otherHistogram.getCountAtIndex(i);

assembly/__tests__/Histogram.spec.ts

@@ -285,7 +285,7 @@ describe("Histogram correcting coordinated omissions", () => {
   });
 });
 
-describe("Histogram add & substract", () => {
+describe("Histogram add & subtract", () => {
   it("should add histograms of same size", () => {
     // given
     const histogram = buildHistogram();
@@ -317,16 +317,33 @@ describe("Histogram add & substract", () => {
     // given
     const histogram = buildHistogram();
     const histogram2 = buildHistogram();
-    histogram.autoResize = true;
-    histogram.recordValue(1000);
-    histogram2.recordValue(42000);
+    histogram.recordCountAtValue(2, 100);
+    histogram2.recordCountAtValue(1, 100);
+    histogram.recordCountAtValue(2, 200);
+    histogram2.recordCountAtValue(1, 200);
+    histogram.recordCountAtValue(2, 300);
+    histogram2.recordCountAtValue(1, 300);
     const outputBefore = histogram.outputPercentileDistribution();
     // when
     histogram.add<Storage<Uint8Array, u8>, u8>(histogram2);
     histogram.subtract<Storage<Uint8Array, u8>, u8>(histogram2);
     // then
     expect(histogram.outputPercentileDistribution()).toBe(outputBefore);
   });
+
+  it("should be equal when another histogram of lower precision is added then subtracted", () => {
+    // given
+    const histogram = new Histogram8(1, 1000000000, 5);
+    const histogram2 = new Histogram8(1, 1000000000, 3);
+    histogram.recordValue(10);
+    histogram2.recordValue(100000);
+    // when
+    const outputBefore = histogram.outputPercentileDistribution();
+    histogram.add<Storage<Uint8Array, u8>, u8>(histogram2);
+    histogram.subtract<Storage<Uint8Array, u8>, u8>(histogram2);
+    // then
+    expect(histogram.outputPercentileDistribution()).toBe(outputBefore);
+  });
 });
 
 describe("Packed Histogram", () => {

src/Histogram.spec.ts

@@ -474,60 +474,35 @@ describe("Histogram add & substract", () => {
     expect(histogram.totalCount).toBe(2);
     expect(Math.floor(histogram.mean / 100)).toBe(215);
   });
-  it("should add histograms of different sizes b", () => {
-    // given
-    const histogram = new Int32Histogram(1, Number.MAX_SAFE_INTEGER, 2);
-    const histogram2 = new Int32Histogram(1, 1024, 2);
-    histogram2.autoResize = true;
-    histogram.recordValue(42000);
-    histogram2.recordValue(1000);
-    // when
-    histogram.add(histogram2);
-    // then
-    expect(histogram.totalCount).toBe(2);
-    expect(Math.floor(histogram.mean / 100)).toBe(215);
-  });
 
-  it("should be equal when another histogram is added then subtracted", () => {
+  it("should be equal when another histogram of lower precision is added then subtracted", () => {
     // given
-    const histogram = new Int8Histogram(1, 1024, 3);
-    const histogram2 = new Int8Histogram(1, 1024, 3);
-    histogram.autoResize = true;
+    const histogram = build({ numberOfSignificantValueDigits: 5 });
+    const histogram2 = build({ numberOfSignificantValueDigits: 3 });
     histogram.recordValue(100);
-    histogram2.recordValue(420);
-    const outputBefore = histogram.outputPercentileDistribution();
+    histogram2.recordValue(42000);
     // when
+    const before = histogram.summary;
     histogram.add(histogram2);
     histogram.subtract(histogram2);
     // then
-    expect(histogram.outputPercentileDistribution()).toBe(outputBefore);
+    expect(histogram.summary).toStrictEqual(before);
   });
 
-  it("should be equal when another wasm histogram is added then subtracted", () => {
+  it("should update percentiles when another histogram of same characteristics is substracted", () => {
     // given
-    const histogram = build({
-      lowestDiscernibleValue: 1,
-      highestTrackableValue: 1024,
-      autoResize: true,
-      numberOfSignificantValueDigits: 5,
-      bitBucketSize: 32,
-      useWebAssembly: true,
-    });
-    const histogram2 = build({
-      lowestDiscernibleValue: 1,
-      highestTrackableValue: Number.MAX_SAFE_INTEGER,
-      numberOfSignificantValueDigits: 5,
-      bitBucketSize: 32,
-      useWebAssembly: true,
-    });
-    histogram.recordValue(1000);
-    histogram2.recordValue(42000);
-    const outputBefore = histogram.outputPercentileDistribution();
+    const histogram = build({ numberOfSignificantValueDigits: 3 });
+    const histogram2 = build({ numberOfSignificantValueDigits: 3 });
+    histogram.recordValueWithCount(100, 2);
+    histogram2.recordValueWithCount(100, 1);
+    histogram.recordValueWithCount(200, 2);
+    histogram2.recordValueWithCount(200, 1);
+    histogram.recordValueWithCount(300, 2);
+    histogram2.recordValueWithCount(300, 1);
     // when
-    histogram.add(histogram2);
     histogram.subtract(histogram2);
     // then
-    expect(histogram.outputPercentileDistribution()).toBe(outputBefore);
+    expect(histogram.getValueAtPercentile(50)).toBe(200);
   });
 });
 

src/JsHistogram.ts

@@ -956,7 +956,10 @@ export abstract class JsHistogram implements Histogram {
         otherHistogram.maxValue
       );
       let otherCount = otherHistogram.getCountAtIndex(otherMaxIndex);
-      this.recordCountAtValue(otherCount, otherHistogram.maxValue);
+      this.recordCountAtValue(
+        otherCount,
+        otherHistogram.valueFromIndex(otherMaxIndex)
+      );
 
       // Record the remaining values, up to but not including the max value:
       for (let i = 0; i < otherMaxIndex; i++) {
@@ -1027,12 +1030,15 @@ export abstract class JsHistogram implements Histogram {
     ) {
       // optim
       // Counts arrays are of the same length and meaning, so we can just iterate and add directly:
+      let observedOtherTotalCount = 0;
       for (let i = 0; i < otherHistogram.countsArrayLength; i++) {
         const otherCount = otherHistogram.getCountAtIndex(i);
         if (otherCount > 0) {
           this.addToCountAtIndex(i, -otherCount);
+          observedOtherTotalCount += otherCount;
         }
       }
+      this.setTotalCount(this.totalCount - observedOtherTotalCount);
     } else {
       for (let i = 0; i < otherHistogram.countsArrayLength; i++) {
         const otherCount = otherHistogram.getCountAtIndex(i);