Stop fetching blob content for diff file metadata code_review_id

Marc Shawrequested to merge
remove-getblobs-content-fetch-diff-files-metadata into master

Diff file metadata requests (diff_files_metadata and the legacy diffs_metadata) were fetching up to 512 KB of blob content per file from Gitaly just to read the blob id used for code_review_id. This change resolves that id from a metadata-only blob lookup (limit 0) when the content blob is not already loaded, so those endpoints no longer download file content they do not need. Paths that already load blob content (the legacy diffs_batch entity and rapid diffs streaming) are unaffected because they reuse the blob that is already memoized.

Rollout order across the epic (&23646): 1. gitaly!9274 (merged) (no flag, Gitaly release + gem and GITALY_SERVER_VERSION bump), then !256363 and its stacked !256402 (shared flag rapid_diffs_metadata_from_stored_diff_files). !256379 (merged) (flag diff_skip_redundant_charset_detection) and !256367 (closed) (flag diff_file_blob_id_metadata_only) are independent and can merge any time. With every flag off, behaviour is unchanged apart from the Feature.enabled? call. This MR: independent, its own flag. Timing on GDK, diff_files_metadata with whitespace shown, median of 8, flag off vs on: 43 files 70 vs 70 ms, 231 files 240 vs 225 ms, 268 files 240 vs 213 ms, 1000 files 397 vs 375 ms. GetBlobs count is unchanged (1 to 5 batches); only the content transfer goes, so the gain is 5 to 10 percent on large diffs. !256402 stops the rapid diffs metadata endpoint calling GetBlobs at all, so this MR mainly helps the legacy diffs page and other stored-diff callers of code_review_id.

Detailed context for AI agents

Problem

  • Projects::MergeRequestsController#diff_files_metadata (app/controllers/concerns/rapid_diffs/resource.rb) renders DiffFileMetadataEntity (app/serializers/diff_file_metadata_entity.rb) for every file in an MR diff. The legacy Projects::MergeRequests::DiffsController#diffs_metadata uses the same entity through DiffsMetadataEntity.
  • The entity exposes code_review_id, which is Gitlab::Diff::File#code_review_id -> #blob_id, previously diff.to_id.presence || diff.from_id.presence || blob&.id.
  • MR diffs are loaded from the database (merge_request_diff_files via MergeRequestDiff#load_diffs), and Gitlab::Git::Diff::SERIALIZE_KEYS does not include to_id/from_id, so every file fell through to blob&.id.
  • blob -> new_blob/old_blob -> fetch_blob -> Blob.lazy(repository, sha, path, blob_size_limit: max_blob_size), where max_blob_size is 512 KB for MR diffs (Gitlab::Diff::FileCollection::MergeRequestDiffBase.max_blob_size). This is the content batch: one Gitaly BlobService#GetBlobs RPC with limit 524288 that downloads up to 512 KB of content per file (about 270 KB in total for a 43-file MR) only to read the blob SHA.

Fix (lib/gitlab/diff/file.rb)

  • blob_id now resolves the SHA as diff.to_id.presence || diff.from_id.presence || new_blob_for_id&.id || old_blob_for_id&.id.
  • new_blob_for_id / old_blob_for_id return the already memoized new_blob / old_blob when it is loaded (checked with strong_memoized?), otherwise they resolve a metadata-only lazy blob: Blob.lazy(repository, sha, path, blob_size_limit: 0), which is the same mechanism Gitlab::Git::Blob.batch_metadata uses (GetBlobs with limit 0 returns size and oid but no data).
  • The metadata lazies are registered in add_blobs_to_batch_loader (called from initialize) next to the existing content lazies, so that when the first file asks for its id the whole collection is fetched in one batched GetBlobs RPC rather than one RPC per file. Blob.lazy keys its BatchLoader batch on [:repository_blobs, repository, blob_size_limit], so the limit-0 batch is separate from the content batch and registering it costs no RPC until it is synced.
  • Precedence (new blob first, then old blob) is unchanged, so the ids are identical to before.
  • Paths that need blob content anyway are unaffected: DiffFileEntity (legacy diffs_batch) exposes blob and viewer before code_review_id, and the rapid diffs streaming loop calls no_preview? and highlighting before rendering the component that reads code_review_id, so in both cases the blobs are already memoized and no metadata RPC is issued.

Verification

Rails runner against the local GDK database, project 2, merge request id 12 with 43 files, rendering DiffFileMetadataEntity over presenter.diffs_resource.raw_diff_files.

Before:

diff_service#diff_stats 275.8ms limit=nil paths=
blob_service#get_blobs 41.1ms limit=524288 paths=43
gitaly_calls=2
first ids: ["8fef2e536cde1774d22e167794a39db9d71c2184", "669d39b072deeb04b13ac921c4281d93499c9384", "b3e4eade5a468d88c8f110ecbeafec89b526dbfe"]
sha1 of all 43 code_review_ids joined: cd276725c9379dc95baeb46c0c8fb4e559222ecc

After:

diff_service#diff_stats 299.8ms limit=nil paths=
blob_service#get_blobs 52.0ms limit=nil paths=43   (limit is 0; protobuf omits the zero default from the recorded request hash)
gitaly_calls=2
first ids: ["8fef2e536cde1774d22e167794a39db9d71c2184", "669d39b072deeb04b13ac921c4281d93499c9384", "b3e4eade5a468d88c8f110ecbeafec89b526dbfe"]
sha1 of all 43 code_review_ids joined: cd276725c9379dc95baeb46c0c8fb4e559222ecc
  • Also confirmed after the change: the blob returned by the metadata batch has data.bytesize == 0 and size == 59, and new_blob is still not memoized after computing the id.
  • Streaming path check (same MR, no_preview? + highlighting then code_review_id per file): one get_blobs with limit 524288 and 43 paths, plus diff_stats and get_info_attributes; no limit-0 request. Legacy batch path check (blob, viewer, then code_review_id per file): one get_blobs with limit 524288 and 43 paths plus get_info_attributes; no limit-0 request. Both digests identical to the metadata path (cd276725c9379dc95baeb46c0c8fb4e559222ecc).

Specs run (bundle exec rspec, one at a time)

  • spec/lib/gitlab/diff/file_spec.rb - 181 examples, 0 failures (three new examples under #code_review_id for the persisted-diff case: metadata-only batch without a content fetch, reuse of an already loaded blob, and fallback to the old blob when the new blob is missing)
  • spec/serializers/diff_file_metadata_entity_spec.rb - 4 examples, 0 failures
  • spec/serializers/diffs_metadata_entity_spec.rb - 6 examples, 0 failures
  • spec/serializers/diff_file_entity_spec.rb - 30 examples, 0 failures
  • spec/requests/projects/merge_requests_controller_spec.rb:575 and spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb fail in the local worktree with asset errors (LoadError: cannot load such file -- sass in vite_helper, and an empty sprite manifest in icons_helper) before any diff code runs; the request spec fails identically with the change reverted (7 examples, 7 failures both ways), so those are environmental to the worktree, not caused by this change. They should pass in CI.
  • rubocop on both changed files: no offenses.

Alternatives considered

  • Persisting to_id/from_id in merge_request_diff_files would remove the Gitaly call entirely, but that table is partitioned and the change needs a migration and backfill, so it is out of scope here and left as a follow-up.
  • Always using the metadata batch (without preferring an already loaded blob) would add a second GetBlobs RPC on the legacy diffs_batch and rapid diffs streaming paths, which already have the content loaded.
  • Not pre-registering the metadata lazies in add_blobs_to_batch_loader would turn the metadata endpoint into one GetBlobs RPC per file.

Out of scope

  • Persisting blob ids in the database (see alternatives above).
  • Any change to the entities or controllers.
  • The sass/asset setup of the local worktree.

Changelog

Commit trailer: Changelog: performance.

Closes #629871 (closed) Epic: &23646

Edited by Marc Shaw

Merge request reports