Skip to content

Clean up completed entries from the LockRegistry - #4545

Open
alecgrieser wants to merge 2 commits into
FoundationDB:mainfrom
alecgrieser:streamline-lock-registry
Open

Clean up completed entries from the LockRegistry#4545
alecgrieser wants to merge 2 commits into
FoundationDB:mainfrom
alecgrieser:streamline-lock-registry

Conversation

@alecgrieser

Copy link
Copy Markdown
Collaborator

This updates the LockRegistry so that it cleans up entries that are no longer necessary. It does this by scheduling a clean up task to delete the entry for the given key from the map, but only if the most recently registered lock for that key still matches the current lock. It is safe to do this when that lock is equivalent to the behavior when there is no lock present, which is to say, when the lock's pending reads and writes are all done, along with the lock's task.

Note that we cannot clean up the lock if the only thing that has happened is that the lock has been released. The reason for this is that if we have a key that has:

  1. A read lock l1
  2. A second read lock l2 with parent l1

So assume l2 is the current lock in heldLocks for a given lock identifier. The l1 and l2 tasks can be executed at the same time, so if l2 removed the entry for the lock in heldLocks when it was done, then we'd have nothing to stop a new write operation l3 from being acquired before l1 had actually finished, breaking our contract. We instead need to wait for l1 to also complete, which is done by having the clean-up operation wait on l2's pending reads (even if l2 is a read lock).

This resolves #4544.

This updates the `LockRegistry` so that it cleans up entries that are no longer necessary. It does this by scheduling a clean up task to delete
the entry for the given key from the map, but only if the most recently registered lock for that key still matches the current lock. It is
safe to do this when that lock is equivalent to the behavior when there is no lock present, which is to say, when the lock's pending reads
and writes are all done, along with the lock's task.

Note that we cannot clean up the lock if the only thing that has happened is that the lock has been released. The reason for this is that if
we have a key that has:

1. A read lock `l1`
1. A second read lock `l2` with parent `l1`

So assume `l2` is the current lock in `heldLocks` for a given lock identifier. The `l1` and `l2` tasks can be executed at the same time,
so if `l2` removed the entry for the lock in `heldLocks` when it was done, then we'd have nothing to stop a new write operation `l3` from
being acquired before `l1` had actually finished, breaking our contract. We instead need to wait for `l1` to also complete, which is done
by having the clean-up operation wait on `l2`'s pending reads (even if `l2` is a read lock).

This resolves FoundationDB#4544.
@alecgrieser alecgrieser added the performance Performance issues label Sep 1, 2026
@alecgrieser
alecgrieser marked this pull request as ready for review September 1, 2026 15:39

@g31pranjal g31pranjal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are pretty well-off with the cleanup proposed in the PR. However, should we add a couple of unit tests around it ?

@alecgrieser

Copy link
Copy Markdown
Collaborator Author

I've added some unit tests here. It ended up being pretty easy, though they depend on the fact that as we're not using whenCompleteAsync, we don't have a race between the task completing and the cleanup callback completing, which is maybe a little subtle. I added some basic smoke tests, and then also some stuff that I've validated would fail if this had cleaned up the top element in the registry upon release (instead of all work below it completing). Though I could see an argument for even more

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

Labels

performance Performance issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LockRegistry can grow without bounds as it does not clean up its keys

2 participants