Cache the previous chunk in Gitlab::HttpIO

Hordur Freyr Yngvasonrequested to merge
fix/http-io-cache-previous-chunk into master

What does this MR do and why?

Gitlab::HttpIO reads remote files (archived CI traces, and remote artifacts opened through GitlabUploader#open) from object storage in 128KB range GETs, caching exactly one chunk at a time (@chunk / @chunk_range).

That single slot is enough for a forward read, which never revisits a chunk. It is not enough for a read that walks backwards, which is what Gitlab::Ci::Trace::Stream#read_backward does in 4KB steps to extract a coverage value from a job trace. A step that straddles a chunk boundary costs three requests instead of one:

  1. tell is in the lower chunk → miss (the cache holds the upper chunk) → GET lower
  2. the same read crosses into the upper chunk → miss (the cache now holds the lower one) → GET upper again
  3. the next step drops back into the lower chunk → miss → GET lower again

Unless the file size happens to be an exact multiple of BUFFER_SIZE, every chunk boundary falls strictly inside some step, so this happens at every boundary and a backward pass costs ~3x the requests and ~3x the bytes of a forward pass over the same data.

Behind the new http_io_previous_chunk_cache feature flag (gitlab_com_derisk, disabled by default), this MR retains the chunk just moved away from — which is exactly the one a backward pass asks for next — so a boundary crossing costs one request again. With the flag disabled, behavior is unchanged.

The cost is one extra chunk buffer (up to 128KB) per open HttpIO, held for the lifetime of the read.

Verification

Measured against the real Gitlab::HttpIO and Gitlab::Ci::Trace::Stream, with Net::HTTP stubbed at the .start boundary to serve and count range requests over an in-memory body:

Read pattern over an 8 MB trace (62 chunks of 128KB) GETs vs. chunk count
Forward, whole file (stream.read) 62 1.0x
Forward, line by line (each_line_with_pos) 62 1.0x
Backward (each_line_backward) 184 3.0x
Backward, with this change 62 1.0x

22.8 MB was fetched from storage to scan that 7.6 MB trace, because every GET pulls a full 128KB regardless of how much of it is used.

The effect is alignment-sensitive, which is why it has gone unnoticed: a file whose size is an exact multiple of BUFFER_SIZE measures 1.0x even without this change, because no step straddles a boundary. Real trace and artifact sizes are arbitrary.

Output is unchanged. spec/lib/gitlab/http_io_spec.rb pins this directly by reading the fixture backwards with the flag on and off and asserting the two results are byte-identical, alongside exact request counts for the cached (one per chunk), uncached (one per chunk plus two per boundary) and forward-read cases.

Relationship to the persistent-connections change

!246918 (merged) (merged) gave Gitlab::HttpIO a persistent connection and explicit timeouts. It removed the connection setup cost of these requests but not the requests themselves: with it, the 184 GETs above are 184 requests over one keep-alive connection rather than 184 connections. This MR is what reduces the request count and the bytes pulled from object storage. The two are complementary and independently flagged.

This branch is rebased on top of it, and the two flags are independent: the request-count assertions in the specs run with both enabled, which is the configuration once both roll out.

How to set up and validate locally

  1. Configure object storage for artifacts (for example, MinIO in GDK) with direct_upload enabled, and run a pipeline with a job large enough to span several 128KB chunks whose coverage: regex never matches.

  2. Once the trace is archived (Ci::ArchiveTraceWorker runs two minutes after the job finishes), extract coverage in a Rails console with the flag off and on, counting object storage requests (MinIO access logs, or mitmproxy):

    build = Ci::Build.find(<id>)

Feature.disable(:http_io_previous_chunk_cache)
build.trace.extract_coverage(build.coverage_regex)   # ~3 requests per 128KB of trace

Feature.enable(:http_io_previous_chunk_cache)
build.trace.extract_coverage(build.coverage_regex)   # ~1 request per 128KB of trace

  3. Confirm the extracted coverage value is the same in both cases.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist.

References

Edited by Hordur Freyr Yngvason

Merge request reports