updateGitMirror: use double checked locking

[byroot]

Currently updateGitMirror acquire the mirror lock before checking for the existence of the revision.

But git rev-parse should be safe to run concurrently, so before locking it's worth checking if the commit is present, which should be safe to do without the lock.

When running builds with large number of jobs, and large server with many agents on them, it's not rare that several agents are looking for the same revision, hence reducing contention on that path is worth it.

Ideally, while we're blocked on the lock, we could occasionally check if the commit has been fetched, but it's a more invasive change.

[isaacsu]

Hiya @byroot, just wanted to acknowledge and thank you for your contribution. I look forward to reviewing it early next week!

[byroot]

Thank you.

On a side note, I'm experimenting with implementing our own checkout, to see if we can optimize this further. I'm not sure why locking is even needed (aside from the clone lock) as IIRC git has its own internal locking when needed.

So I'm going to experiment with no locking at all, and I'll report back if it's successful.

[byroot]

I found this comment suspect so I dug a bit in the history, look like the agent used to check before the lock a few years ago: https://github.com/byroot/agent/blame/2a581a13e7f63e69651f2bbe689e44db6d33e074~1/internal/job/executor.go#L951

It seems it was removed in #3789, but it's not clear to me why.

[isaacsu]

As it turns out, the reason we removed this "snapshot an existing commit without a lock" is because it exposes us to the following race condition:

1. Job A sees that a commit that it requires already exists in the mirror and starts snapshotting it without holding a lock.
2. Concurrently, Job B triggers a fetch on the mirror because the commit that it requires doesn't exist in the mirror. This fetch can trigger a git gc, potentially corrupting Job A's snapshot process.

Your use-case where a large number of jobs are snapshotting the same commit is valid though.

Maybe there's something in the region of a commit-aware lock, such that jobs requesting the same commit can safely snapshot concurrently (no risk of corruption from gc), while jobs seeking a different commit can wait for a lock (status quo).

[byroot]

This fetch can trigger a git gc

Interesting. I'll make sure to disable auto-gc. It makes for unpredictable performance anyway.

Maybe there's something in the region of a commit-aware lock,

Yes, that was my backup idea :)

[byroot]

Another avenue (which can also be complementary) is to use flock's RWLock ability.

If the concern is that a fetch could cause a GC break concurrent clones, then clones could acquire the lock with LOCK_SH. This way only fetch operations would use LOCK_EX:

if !mirror.has_commit?(SHA)
  flock.lock_exclusive(SHA) do
    if !mirror.has_commit?(SHA) # Check again in case another process did fetch it for us
      flock.lock_exclusive do
        mirror.fetch(SHA)
      end
    end
  end
end

# Ensure we're not cloning during a concurrent `fetch`
# but still allow concurrent clones.
flock.lock_shared do
  do_local_clone
end

[isaacsu]

Another avenue (which can also be complementary) is to use flock's RWLock ability.

Yeah, this could totally work and feels like a complete solution. Curious to hear how you see this as merely complementary.

[byroot]

Curious to hear how you see this as merely complementary.

Well, I said can :)

My hunch (we may be totally wrong) is that even fetching a commit that is already present still increase contention on the mirror, hence it could be beneficial to park the agents that wish to fetch a commit that is already being fetched (or about to) on a secondary lock.

[isaacsu]

even fetching a commit that is already present still increase contention on the mirror

Assuming you meant "snapshotting a commit …", what contention do you see arising on the mirror? The only one I can think of is if somehow git would report a commit as "present", but partially GC'ed and thus requiring a fetch, but that feels kind of bad if true.

I tried to game out the exclusive/shared lock idea below but with the shared lock annotated with a commit sha (I don't think your sketch above does this, but I could've misread).

I think it will allow concurrent snapshots of a present commit with no risk of corruption from a concurrent fetch/gc.

What do you think?

---

• Job A wants commit 001a
• Job B wants commit 002b
• Job C wants commit 002b
• Job D wants commit 004d
• Job E wants commit 002b

None of the commits currently exist in the mirror.

There are two locks:

• LOCK_EX (exclusive lock)
• LOCK_SH(commit) (commit aware shared lock)

Sequence:

1. Job A sees that 001a is absent (in the mirror) and both locks are free, so it takes LOCK_EX and fetches 001a.
   • Jobs B-E all require commits that are absent, so they wait on LOCK_EX.
2. Job A finishes and releases LOCK_EX.
3. Job B sees that 002b is absent and both locks are free, so it takes LOCK_EX and fetches 002b.
   • Jobs C-E all require commits that are absent, so they wait on LOCK_EX.
4. Job B finishes and releases LOCK_EX.
5. Job C sees that 002b is present and both locks are free, so it takes LOCK_SH(002b) and snapshots 002b.
   • Job D requires 004d that is absent, so it waits on LOCK_SH(002b).
   • Job E requires 002b that is present and LOCK_SH(002b) is active, so it also takes LOCK_SH(002b) and snapshots 002b concurrently.
6. Job C and Job E finish and release LOCK_SH(002b).
7. Job D sees that 004d is absent and both locks are free, so it takes LOCK_EX and fetches 004d.
8. Job D finishes and releases LOCK_EX.

[byroot]

Assuming you meant "snapshotting a commit …",

By snapshotting you mean running git fetch origin SHA in the local clone with the shared mirror as reference?

what contention do you see arising on the mirror?

By contention I just mean that the Prepare working directory performance is very inconsistent. It ranges from 4s to 24s, which I attribute to contention on the lock/mirror.

For full context, we previously had the update-mirror setting disabled (from several years ago), and since when a node boot it download some daily snapshots of the mirrors from S3, unless a node stayed online for a few days, the prepare working directory performance was very good (somewhat stable 4-5s).

However we noticed a lot of traffic to github (very costly because EC2), so we've now re-enabled the mirror updating, which did bring the github traffic down, but that's where we noticed the performance becoming much less stable.

What do you think?

I think you can achieve better, with just a single lock that is taken as EX when fetching in the mirror, and as SH when "snaposhotting".

Let me replicate your scenario:

---

• Job A wants commit 001a
• Job B wants commit 002b
• Job C wants commit 002b
• Job D wants commit 004d
• Job E wants commit 001a

None of the commits currently exist in the mirror.

There is a single locks:

• To fetch the lock is acquired with LOCK_EX (exclusive lock)
• To snapshot, the lock is acquired with LOCK_SH (shared lock)

Sequence:

1. Job A sees that 001a is absent (in the mirror) and the lock is free, so it takes LOCK_EX and fetches 001a.
   • Jobs B-E all require commits that are absent, so they wait on LOCK_EX.
2. Job A finishes and releases LOCK_EX.
3. Job B sees that 002b is absent and the lock is free, so it takes LOCK_EX and fetches 002b.
   • Jobs C-E all require commits that are absent, so they wait on LOCK_EX.
4. Job B finishes and releases LOCK_EX.
5. Job C sees that 002b is present and the lock is free, so it takes LOCK_SH and snapshots 002b.
   • Job D requires 004d that is absent, so it try to acquire LOCK_EX and waits on LOCK_SH.
   • Job E requires 001a that is present so it also takes LOCK_SH and snapshots 001b concurrently.
6. Job C and Job E finish and release LOCK_SH.
7. Job D sees that 004d is absent the locks is free, so it takes LOCK_EX and fetches 004d.
8. Job D finishes and releases LOCK_EX.

[lox]

Btw @byroot you might be interested in the git caching work we are doing in https://github.com/buildkite/content-cache.

[isaacsu]

By snapshotting you mean running git fetch origin SHA in the local clone with the shared mirror as reference?

Ah yes. I overindexed on the "--mirror of a --mirror" process in snapshotMirror when clean-checkout is enabled and missed the early return. In this scenario, I figured there would be less chance of contention since the local clone is interacting with an intermediate "snapshot" of the shared mirror.

But when clean-checkout is disabled, the local clone indeed interacts directly with the shared mirror, increasing the chances of contention.

I think you can achieve better

Ah I see it now - if all the commits requested by the jobs are present on the shared mirror and there is no fetching in flight (LOCK_EX is free), then all the jobs can read concurrently (under LOCK_SH). Thanks for spelling it out 🙏

I hope to spend a bit more time on this next week. Will keep you posted!

[lox]

Just catching up here and figuring out the intent behind the snapshotMirror history introduced in #3789. My read is that the snapshot turns the mutable shared mirror into a stable per-job object store: its hardlinks keep the required objects alive even if the shared mirror is later repacked or garbage-collected. That only works if the mirror remains stable while the snapshot itself is being created. @DrJosh9000 is that correct?

I think the right model is one repo-wide RW lock, not a per-commit lock. The resource being protected is the mirror, and operations such as repack or GC can rewrite packs containing many commits. Jobs requesting any already-present commit take LOCK_SH across both hasGitCommit and snapshotMirror. Shared locks are compatible, so those jobs can snapshot concurrently even when they want different commits. Initialization, fetch, repack, GC, and replacement take LOCK_EX.

On a cache miss, the job can drop LOCK_SH, take LOCK_EX, recheck, fetch if necessary, and then either snapshot under the exclusive lock or retry through the shared path.

The change currently proposed checks for the commit and calls snapshotMirror without holding a lock. Another job can acquire the exclusive lock and mutate the mirror after the check or while git clone --mirror is reading it. That won't fail every time, but it can produce an incomplete snapshot or missing-object failures, so I don't think the unlocked fast path is safe.

Keen on @DrJosh9000's thoughts too, I imagine there was a ton of additional context behind the original change.

[byroot]

The change currently proposed checks for the commit and calls snapshotMirror without holding a lock. Another job can acquire the exclusive lock and mutate the mirror after the check or while git clone --mirror is reading it.

Yes, we established that in #4074 (comment)

I haven't updated the PR accordingly, but as we both mentioned, a way to reduce contention a bit is to use a shared lock during "snapshoting". I can try to implement that is there is interest, but given the discussion is still ongoing that maybe isn't worth it yet.

[byroot]

the job can drop LOCK_SH, take LOCK_EX,

It's a detail, but in theory it should be able to take LOCK_EX directly in one step:

Subsequent flock() calls on an already locked file will convert an existing lock to the new lock mode.

[byroot]

So, after trying my custom checkout more extensively, and more importantly instrumenting individual git commands rather than the full step, it turns out that lock contention isn't the biggest bottleneck.

What's really causing the Prepare Working Directory step to vary between 5s and 25s is the git checkout -f command. It routinely takes over 10 seconds on busy nodes, which on less busy ones it's just a couple seconds.

So it seems we're experiencing some IO bottleneck....

I still think it'd be nice to keep the lock less contended, but that alone wouldn't solve our issue 😢