Parallel compression and bsdiff

[jorisdobbelsteen]

I have a similar performance desire as in issue #2845, except for the compression part.

This change adds a way that bsdiff and lzma compression for static-delta can be run on multiple cores.
Verification was done with sanitisers, and the thread sanitiser; the output should be byte identical.

Looking for input on this feature, since I uses claude and before I invest significant time for further clean up of any code.

[openshift-ci]

Hi @jorisdobbelsteen. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[gemini-code-assist]

Code Review

This pull request introduces parallel compression and bsdiff processing for static delta generation in libostree. It adds a new --threads option to the static-delta generate command, allowing users to specify the number of worker threads or use keywords like 'auto' to utilize all available CPUs. The implementation introduces thread pools for both compression and bsdiff tasks, ensuring that the output remains byte-identical to the serial path. The review identified potential race conditions in error propagation and suggested improvements for fail-fast behavior in the bsdiff task submission process.

[gemini-code-assist]

Using g_steal_pointer here introduces a race condition. If the producer thread is woken up by a worker's failure, it steals the error from the pool, making cp->first_error NULL. If the producer then continues and calls submit_finish_part again (e.g., for the final part), it will not see the error and may incorrectly submit more work. This can lead to unpredictable behavior in error scenarios.

You should use g_error_copy to propagate the error without clearing the error state from the pool. This ensures that subsequent checks against cp->first_error will correctly detect the failed state.

      g_propagate_error (error, g_error_copy (cp->first_error));

[jorisdobbelsteen]

Thanks. There's no data race here: first_error is only read/written under cp->mutex, there's a single producer thread, and every caller of submit_finish_part() returns immediately on the first failure — so the "producer continues and submits the final part" scenario can't occur, and drain_compress_pool() is never reached on that path (no double-report).

That said, the change pointed at a real smell: g_steal_pointer consumed the error and cleared the "pool has failed" flag that queued workers rely on to short-circuit. I've reworked it so the pool owns first_error for its lifetime and propagates a copy (freed once in worker_pool_clear()), which is what the bsdiff pool already did — so both pools are now consistent and the flag stays valid. Done in submit_finish_part() and drain_compress_pool(), with comments explaining the ownership model.

[gemini-code-assist]

This section could be improved to fail-fast if an error has already occurred in another worker. Currently, the producer will continue to enqueue bsdiff tasks even if the pool is in a failed state. While workers will short-circuit, stopping the producer earlier is more efficient and consistent with the error handling in the compression pool.

Consider checking for bp->first_error before incrementing inflight and pushing to the pool.

  g_mutex_lock (&bp->mutex);
  if (bp->first_error)
    {
      g_propagate_error (error, g_error_copy (bp->first_error));
      g_mutex_unlock (&bp->mutex);
      return FALSE;
    }
  bp->inflight++;
  g_mutex_unlock (&bp->mutex);

[jorisdobbelsteen]

Applied — submit_bsdiff_task() now checks bp->first_error under the mutex before incrementing inflight and returns early (propagating a copy) if another worker has already failed, mirroring submit_finish_part(). The already-submitted tasks are still reaped by drain_bsdiff_pool() on the error path.

[jorisdobbelsteen]

Pushed an updated revision. Summary of changes since the last push:

• clang-format: fixed the formatting violations (the only CI failure actually caused by this PR).
• Error propagation: reworked the compress pool's first_error handling. It now follows a single copy-and-own model shared with the bsdiff pool — the pool owns first_error for its whole lifetime, callers receive a g_error_copy, and it's freed exactly once in worker_pool_clear(). This keeps the "pool has failed" short-circuit flag valid for in-flight workers (previously g_steal_pointer cleared it) and matches existing prior art in the tree (ostree-repo.c, ostree-mutable-tree.c, ostree-fetcher-soup.c). Added comments documenting the ownership invariant.
• bsdiff fail-fast: submit_bsdiff_task() now bails early if another worker has already failed, mirroring submit_finish_part().

Design note for maintainers: this introduces GThreadPool + GMutex/GCond, which is a new concurrency primitive for src/ — the established async idiom here is GTask/GMainContext (e.g. ostree-repo-pull.c). I went with a thread pool because the work is purely CPU-bound (LZMA compression and bsdiff), where the GTask/GMainContext model would be awkward; the GMutex/GCond building blocks themselves are already used elsewhere (ostree-async-progress.c, ostree-fetcher-soup.c). Flagging it explicitly in case you'd prefer a different approach.

Scope confirmation: no public API/ABI change — no installed header or .sym file is touched, ostree_repo_static_delta_generate()'s signature is unchanged, and --threads is exposed only via the CLI plus a new key in the existing params a{sv} dict.

The remaining red checks are unrelated CI-infra failures (SIGSEGV during the CoreOS-buildroot cargo/dnf setup, and apt/packit flakes) — the jobs that actually build this change (build-git-libostree, all Debian/Ubuntu/Fedora builds, clang-format) pass.