Fix range download: use real seek instead of read-and-discard#3044
Fix range download: use real seek instead of read-and-discard#3044clayly wants to merge 1 commit intoeclipse-hawkbit:masterfrom
Conversation
FileStreamingUtil.copyStreams called IOUtils.skipFully(from, start), which reads start bytes through a 2KB scratch buffer. Combined with ArtifactStream not overriding skip(long), a Range request at offset 600MB on an 800MB artifact made the server read+discard 600MB before serving any payload. With 80 concurrent devices this saturated CPU. Fix: - ArtifactStream.skip(long) now delegates to the wrapped stream so a FileInputStream can lseek(2). Non-seekable backends (CipherInputStream for encrypted artifacts, S3 streams) keep their existing behaviour. - FileStreamingUtil.copyStreams uses InputStream.skipNBytes(start) instead of IOUtils.skipFully so the call chain reaches the underlying skip(). JMH (single thread, 600MB offset, 1MB read): 27.21 ms -> 0.034 ms (800x). Real stack (80 parallel curl, 1MB range at 600MB offset): avg 728 ms -> 28 ms (26x), p99 966 ms -> 54 ms. Adds JMH test-scope dep and FileStreamingBenchmark/BufferSizeBenchmark for regression detection. Both gated on -Dperf=true so default test runs stay fast.
|
Thanks @clayly for taking the time to contribute to hawkBit! We really appreciate this. Make yourself comfortable while I'm looking for a committer to help you with your contribution. |
|
A note on It was written during the seek-fix investigation to confirm that the existing
Conclusion: keep Happy to drop it from this PR and ship it as a separate "perf benchmark" PR if reviewers prefer to keep the fix change-set minimal. |
Summary
FileStreamingUtil.copyStreamscallsIOUtils.skipFully(from, start)to position the artifact stream at the requested range start. Apache Commons IO'sskipFullyreadsstartbytes through a 2KB scratch buffer and discards them — it never delegates toInputStream.skip(). Even if it did,ArtifactStreamdoes not overrideskip(long), so the defaultInputStream.skipimplementation (which also reads through a scratch buffer) would be used.For an 800 MB artifact and a device requesting
bytes=600000000-..., the server reads and discards 600 MB per request before serving any payload. With 80 concurrent devices issuing range requests during a rollout, the host saturates CPU on memcpy and disk I/O.Fix
ArtifactStream.skip(long)now delegates to the wrapped stream so aFileInputStreamcan issue anlseek(2)(O(1) seek). Non-seekable backends (CipherInputStreamfor encrypted artifacts, S3 streams) keep their existing read-and-discard behavior — no regression.FileStreamingUtil.copyStreamsusesInputStream.skipNBytes(start)(Java 12+ stdlib) instead ofIOUtils.skipFully.skipNBytescallsskip()first and only falls back toread()ifskip()returned 0.Performance
JMH (single thread, ms/op, lower is better):
skipFullyskipNBytesReal stack (80 parallel curl, 1 MB range at 600 MB offset, hawkbit + Postgres on warm SSD):
Numbers above are conservative — production with cold cache or HDD-backed
FileArtifactStoragewill see a larger gain (no need to read 600 MB from disk to throw it away).Test plan
FileStreamingUtilTest(2 tests) — passDdiArtifactDownloadTestincludingrangeDownloadArtifact(4 tests) — passFileStreamingBenchmark,BufferSizeBenchmark, gated by-Dperf=true) — run viamvn -pl hawkbit-rest/hawkbit-rest-core test -Dtest=FileStreamingBenchmark -Dperf=trueCompatibility
InputStream.skipNBytesis available since Java 12; hawkbit baseline is Java 17 — safe.ArtifactStream.skipoverride is additive — no change to existing callers.skip(long)decides behavior. Either it natively seeks (gain) or reads-and-discards (current behavior). No regression path.