feat(histogram): Adding support for optimized sum() for delta-histograms - #2187
Open
sandeep6189 wants to merge 2 commits into
Open
feat(histogram): Adding support for optimized sum() for delta-histograms#2187sandeep6189 wants to merge 2 commits into
sandeep6189 wants to merge 2 commits into
Conversation
sandeep6189
force-pushed
the
delta-histogram-sum-optimizations
branch
from
April 5, 2026 01:52
1483121 to
9a8c6b8
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces an optimized native (Rust/JNI) implementation for summing delta-histogram bucket values, intended to speed up queries like sum(rate(delta_hist[window])) by avoiding per-row JVM decompression overhead.
Changes:
- Add
DeltaHistogramReader(used forSUBTYPE_H_SIMPLE) that can routesum(start,end)through a new JNI call when enabled. - Implement
SimdNativeMethods.histogramBatchSumin Rust (simd_vectors.rs) to walk histogram sections and batch-unpack+accumulate NibblePack delta blobs. - Add extensive unit tests and a new end-to-end JMH benchmark for delta-histogram rate queries; wire up config toggle via
GlobalConfig.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| jmh/src/main/scala/filodb.jmh/DeltaHistogramE2EBenchmark.scala | Adds an end-to-end benchmark to compare SIMD/native vs JVM paths for delta-histogram sum(rate()) queries. |
| core/src/test/scala/filodb.memory/format/vectors/HistogramVectorTest.scala | Adds correctness tests for native vs JVM histogram summation and Rust-vs-Scala NibblePack unpacking. |
| core/src/rust/filodb_core/src/simd_vectors.rs | Implements the JNI batch-sum routine and NibblePack delta unpacking in Rust. |
| core/src/main/scala/filodb.memory/format/vectors/SimdNativeMethods.scala | Adds a runtime toggle and JNI method declaration for histogram batch sum. |
| core/src/main/scala/filodb.memory/format/vectors/HistogramVector.scala | Routes SUBTYPE_H_SIMPLE readers to DeltaHistogramReader and adds the native-accelerated sum() override. |
| core/src/main/scala/filodb.core/GlobalConfig.scala | Enables the new optimization via config (filodb.simd.delta-histogram-sum-optimized-enabled). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| in_word = read_i64_safe(input.add(buf_index), input_len - buf_index); | ||
| buf_index += 8; | ||
| if remaining < num_bits { | ||
| out_word |= (in_word << remaining as i64) & mask; |
Comment on lines
+382
to
+405
| for _local_i in 0..sect_num_elems { | ||
| if blob_ptr >= sect_data_end || blob_ptr >= vector_end { break; } | ||
|
|
||
| let blob_base = blob_ptr as *const u8; | ||
| let blob_len = unsafe { read_u16_at(blob_base, 0) } as usize; | ||
|
|
||
| if global_elem >= start_row && global_elem <= end_row { | ||
| for v in values.iter_mut() { *v = 0; } | ||
|
|
||
| let ok = unsafe { | ||
| nibble_unpack_delta((blob_ptr + 2) as *const u8, blob_len, &mut values, num_buckets) | ||
| }; | ||
| if ok { | ||
| for b in 0..num_buckets { | ||
| unsafe { *out.add(b) += values[b] as f64; } | ||
| } | ||
| summed += 1; | ||
| } | ||
| } | ||
|
|
||
| blob_ptr += 2 + blob_len; | ||
| global_elem += 1; | ||
| if global_elem > end_row { break; } | ||
| } |
Comment on lines
+678
to
+705
| override def sum(start: Int, end: Int): MutableHistogram = { | ||
| // Native path: only when enabled AND vector is in off-heap native memory | ||
| if (SimdNativeMethods.deltaHistogramSumEnabled && numBuckets > 0 && | ||
| (acc eq MemoryReader.nativePtrReader)) { | ||
| try { | ||
| val outAddr = UnsafeUtils.unsafe.allocateMemory(numBuckets.toLong * 8) | ||
| try { | ||
| val summed = SimdNativeMethods.histogramBatchSum( | ||
| histVect2.addr, start, end, outAddr, numBuckets) | ||
| if (summed >= 0) { | ||
| val outValues = new Array[Double](numBuckets) | ||
| cforRange { 0 until numBuckets } { i => | ||
| outValues(i) = UnsafeUtils.unsafe.getDouble(outAddr + i.toLong * 8) | ||
| } | ||
| return MutableHistogram(buckets, outValues) | ||
| } | ||
| // summed < 0 means native error — fall through to super | ||
| } finally { | ||
| UnsafeUtils.unsafe.freeMemory(outAddr) | ||
| } | ||
| } catch { | ||
| case e: Exception => | ||
| logger.warn(s"Native histogramBatchSum failed, falling back to JVM sum: ${e.getMessage}") | ||
| } | ||
| } | ||
| // Fallback: original JVM implementation | ||
| super.sum(start, end) | ||
| } |
Comment on lines
+833
to
+834
| (0 until 8).map(b => ((i + b) % 16).toLong).toArray. | ||
| scanLeft(0L)(_ + _).tail.toArray // make cumulative |
Comment on lines
+438
to
+447
| it("native sum should match JVM sum for geometric bucket histograms") { | ||
| val appender = HistogramVector.appending(memFactory, 1024) | ||
| rawLongBuckets.foreach { rawBuckets => | ||
| BinaryHistogram.writeDelta(bucketScheme, rawBuckets, buffer) | ||
| appender.addData(buffer) shouldEqual Ack | ||
| } | ||
| val reader = appender.reader.asHistReader | ||
|
|
||
| SimdNativeMethods.deltaHistogramSumEnabled = false | ||
| val jvmSum = reader.sum(0, rawHistBuckets.length - 1) |
| * Uses GatewayServer sharding pipeline + TestTimeseriesProducer.genHistogramData | ||
| * for realistic ingestion (proper sharding, multi-shard, stream ingestion). | ||
| * | ||
| * Setup: 100 delta-histogram series, 7 days at 10s interval, 20 buckets. |
Comment on lines
+680
to
+697
| if (SimdNativeMethods.deltaHistogramSumEnabled && numBuckets > 0 && | ||
| (acc eq MemoryReader.nativePtrReader)) { | ||
| try { | ||
| val outAddr = UnsafeUtils.unsafe.allocateMemory(numBuckets.toLong * 8) | ||
| try { | ||
| val summed = SimdNativeMethods.histogramBatchSum( | ||
| histVect2.addr, start, end, outAddr, numBuckets) | ||
| if (summed >= 0) { | ||
| val outValues = new Array[Double](numBuckets) | ||
| cforRange { 0 until numBuckets } { i => | ||
| outValues(i) = UnsafeUtils.unsafe.getDouble(outAddr + i.toLong * 8) | ||
| } | ||
| return MutableHistogram(buckets, outValues) | ||
| } | ||
| // summed < 0 means native error — fall through to super | ||
| } finally { | ||
| UnsafeUtils.unsafe.freeMemory(outAddr) | ||
| } |
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.
Pull Request checklist
Current behavior : (link exiting issues here : https://help.github.com/articles/basic-writing-and-formatting-syntax/#referencing-issues-and-pull-requests)
New behavior :
BREAKING CHANGES
If this PR contains a breaking change, please describe the impact and migration
path for existing applications.
If not please remove this section.
Breaking changes may include:
Other information: