Skip to content

Fix range download: use real seek instead of read-and-discard#3044

Open
clayly wants to merge 1 commit intoeclipse-hawkbit:masterfrom
clayly:perf/range-seek-fix
Open

Fix range download: use real seek instead of read-and-discard#3044
clayly wants to merge 1 commit intoeclipse-hawkbit:masterfrom
clayly:perf/range-seek-fix

Conversation

@clayly
Copy link
Copy Markdown
Contributor

@clayly clayly commented May 2, 2026

Summary

FileStreamingUtil.copyStreams calls IOUtils.skipFully(from, start) to position the artifact stream at the requested range start. Apache Commons IO's skipFully reads start bytes through a 2KB scratch buffer and discards them — it never delegates to InputStream.skip(). Even if it did, ArtifactStream does not override skip(long), so the default InputStream.skip implementation (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

  1. ArtifactStream.skip(long) now delegates to the wrapped stream so a FileInputStream can issue an lseek(2) (O(1) seek). Non-seekable backends (CipherInputStream for encrypted artifacts, S3 streams) keep their existing read-and-discard behavior — no regression.
  2. FileStreamingUtil.copyStreams uses InputStream.skipNBytes(start) (Java 12+ stdlib) instead of IOUtils.skipFully. skipNBytes calls skip() first and only falls back to read() if skip() returned 0.

Performance

JMH (single thread, ms/op, lower is better):

offset legacy skipFully fixed skipNBytes speedup
0 MB 0.034 0.035 1x
100 MB 5.43 0.034 155x
400 MB 20.65 0.034 607x
600 MB 27.21 0.034 800x

Real stack (80 parallel curl, 1 MB range at 600 MB offset, hawkbit + Postgres on warm SSD):

metric before after speedup
avg 728 ms 28 ms 26x
p50 748 ms 27 ms 27x
p95 943 ms 49 ms 19x
p99 966 ms 54 ms 18x

Numbers above are conservative — production with cold cache or HDD-backed FileArtifactStorage will see a larger gain (no need to read 600 MB from disk to throw it away).

Test plan

  • FileStreamingUtilTest (2 tests) — pass
  • DdiArtifactDownloadTest including rangeDownloadArtifact (4 tests) — pass
  • JMH benches added (FileStreamingBenchmark, BufferSizeBenchmark, gated by -Dperf=true) — run via mvn -pl hawkbit-rest/hawkbit-rest-core test -Dtest=FileStreamingBenchmark -Dperf=true
  • Verify on a customer deployment with encrypted artifacts (CipherInputStream path keeps read-discard, so no behavioral change there)

Compatibility

  • InputStream.skipNBytes is available since Java 12; hawkbit baseline is Java 17 — safe.
  • ArtifactStream.skip override is additive — no change to existing callers.
  • For non-seekable backends, the wrapped stream's own skip(long) decides behavior. Either it natively seeks (gain) or reads-and-discards (current behavior). No regression path.

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.
@hawkbit-bot
Copy link
Copy Markdown

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.
Please make sure you read the contribution guide and signed the Eclipse Contributor Agreement (ECA).

@clayly
Copy link
Copy Markdown
Contributor Author

clayly commented May 3, 2026

A note on BufferSizeBenchmark since it appears in this PR without a corresponding BUFFER_SIZE change:

It was written during the seek-fix investigation to confirm that the existing 0x2000 (8 KB) constant is still a sensible default after the seek fix removes the dominant per-request CPU cost. Findings:

  • JMH micro (in-process, CountingSink consumer): all sizes 8 KB-1 MB land within margin of error (e.g. single-thread 100 MB payload: 8 KB = 24.0 ± 0.99 ms, 64 KB = 23.1 ± 0.88, 256 KB = 22.7 ± 0.08, 1 MB = 23.5 ± 1.24). Inconclusive in isolation.
  • End-to-end on real Tomcat (80 parallel curl, 100 MB range): 8 KB read buffer averaged 462 ms/request, 256 KB averaged 2 737 ms/request — ~6x worse with the larger read buffer. Cause: response.setBufferSize(...) in FileStreamingUtil caps the Tomcat output buffer at the same 8 KB; a 256 KB read forces 32 sub-flushes through Tomcat's internal 8 KB window, adding memcpy without reducing flush count, plus 80 × 256 KB = ~20 MB of per-request heap pressure.

Conclusion: keep BUFFER_SIZE at 8 KB. The bench is retained in this PR as a regression guard so anyone proposing to bump the constant in the future can quickly run -Dperf=true and see the JMH/end-to-end discrepancy themselves before changing it.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants