Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

db: avoid acquiring DB.mu in the commit pipeline #2680

Closed
jbowens opened this issue Jun 26, 2023 · 0 comments · Fixed by #3602
Closed

db: avoid acquiring DB.mu in the commit pipeline #2680

jbowens opened this issue Jun 26, 2023 · 0 comments · Fixed by #3602
Labels
A-storage T-storage
Projects
Storage

Comments

@jbowens
Copy link
Collaborator

jbowens commented Jun 26, 2023

Currently during batch prepare, a batch acquires the commit mutex. It then allocates sequence numbers for its keys. Then, in commitWrite it locks the global database mutex in order to grab the current mutable memtable:

	d.mu.Lock()

	var err error
	if !b.ingestedSSTBatch {
		// Batches which contain keys of kind InternalKeyKindIngestSST will
		// never be applied to the memtable, so we don't need to make room for
		// write. For the other cases, switch out the memtable if there was not
		// enough room to store the batch.
		err = d.makeRoomForWrite(b)
	}

	if err == nil && !d.opts.DisableWAL {
		d.mu.log.bytesIn += uint64(len(repr))
	}

	// Grab a reference to the memtable while holding DB.mu. Note that for
	// non-flushable batches (b.flushable == nil) makeRoomForWrite() added a
	// reference to the memtable which will prevent it from being flushed until
	// we unreference it. This reference is dropped in DB.commitApply().
	mem := d.mu.mem.mutable

	d.mu.Unlock()

This can contribute to high tail latencies (#2646). All batch commits become stalled, waiting for the goroutine holding DB.mu to complete.

We could avoid this mutex acquisition in the common case where there's room in the existing memtable for the current batch. The committing goroutine can load the mutable memtable through an atomic load. Memtable reservation can also be performed through lock-free operations. If reservation finds that there is not sufficient space in the memtable, it falls back into acquiring d.mu and entering makeRoomForWrite. There are instances where we rotate the memtable before it's full. In these instances, makeRoomForWrite can set the mutable memtable to nil, reflecting that a rotation is in progress and incoming writers must queue in makeRoomForWrite. A version of this was hacked together in ef6e59b.
@blathers-crl blathers-crl bot added this to Incoming in Storage Jun 26, 2023
@jbowens jbowens moved this from Incoming to Backlog in Storage Jun 26, 2023
@jbowens jbowens mentioned this issue Jul 13, 2023
Closed
jbowens added a commit to jbowens/pebble that referenced this issue May 9, 2024
@jbowens
db: avoid acquiring DB.mu during batch commit, unless rotating memtable
123b307 
Previously, DB.commitWrite would acquire the DB.mu before calling
makeRoomForWrite to ensure the current memtable had sufficient space for the
batch's contents. When DB.commitWrite is called, the caller already holds the
commit pipeline's mutex providing mutual exclusion with other committers and,
crucially, preventing the concurrent rotation of the memtable.

This commit adjusts DB.commitWrite to attempt reserving space in the current
mutable memtable before acquiring DB.mu. In the common case, if successful,
there is no need to acquire DB.mu. If unsuccessful, the committer falls back to
acuqiring DB.mu and rotating the memtable through calling makeRoomForWrite.

Close cockroachdb#2680.
@jbowens jbowens mentioned this issue May 9, 2024
Merged
jbowens added a commit to jbowens/pebble that referenced this issue May 9, 2024
@jbowens
db: avoid acquiring DB.mu during batch commit, unless rotating memtable
b7cb5a4 
Previously, DB.commitWrite would acquire the DB.mu before calling
makeRoomForWrite to ensure the current memtable had sufficient space for the
batch's contents. When DB.commitWrite is called, the caller already holds the
commit pipeline's mutex providing mutual exclusion with other committers and,
crucially, preventing the concurrent rotation of the memtable.

This commit adjusts DB.commitWrite to attempt reserving space in the current
mutable memtable before acquiring DB.mu. In the common case, if successful,
there is no need to acquire DB.mu. If unsuccessful, the committer falls back to
acuqiring DB.mu and rotating the memtable through calling makeRoomForWrite.

Close cockroachdb#2680.
jbowens added a commit to jbowens/pebble that referenced this issue May 12, 2024
@jbowens
db: avoid acquiring DB.mu during batch commit, unless rotating memtable
83a0230 
Previously, DB.commitWrite would acquire the DB.mu before calling
makeRoomForWrite to ensure the current memtable had sufficient space for the
batch's contents. When DB.commitWrite is called, the caller already holds the
commit pipeline's mutex providing mutual exclusion with other committers and,
crucially, preventing the concurrent rotation of the memtable.

This commit adjusts DB.commitWrite to attempt reserving space in the current
mutable memtable before acquiring DB.mu. In the common case, if successful,
there is no need to acquire DB.mu. If unsuccessful, the committer falls back to
acuqiring DB.mu and rotating the memtable through calling makeRoomForWrite.

Close cockroachdb#2680.
jbowens added a commit to jbowens/pebble that referenced this issue May 12, 2024
@jbowens
db: avoid acquiring DB.mu during batch commit, unless rotating memtable
3795cf7 
Previously, DB.commitWrite would acquire the DB.mu before calling
makeRoomForWrite to ensure the current memtable had sufficient space for the
batch's contents. When DB.commitWrite is called, the caller already holds the
commit pipeline's mutex providing mutual exclusion with other committers and,
crucially, preventing the concurrent rotation of the memtable.

This commit adjusts DB.commitWrite to attempt reserving space in the current
mutable memtable before acquiring DB.mu. In the common case, if successful,
there is no need to acquire DB.mu. If unsuccessful, the committer falls back to
acuqiring DB.mu and rotating the memtable through calling makeRoomForWrite.

Close cockroachdb#2680.
jbowens added a commit to jbowens/pebble that referenced this issue May 13, 2024
@jbowens
db: avoid acquiring DB.mu during batch commit, unless rotating memtable
7d63c4c 
Previously, DB.commitWrite would acquire the DB.mu before calling
makeRoomForWrite to ensure the current memtable had sufficient space for the
batch's contents. When DB.commitWrite is called, the caller already holds the
commit pipeline's mutex providing mutual exclusion with other committers and,
crucially, preventing the concurrent rotation of the memtable.

This commit adjusts DB.commitWrite to attempt reserving space in the current
mutable memtable before acquiring DB.mu. In the common case, if successful,
there is no need to acquire DB.mu. If unsuccessful, the committer falls back to
acuqiring DB.mu and rotating the memtable through calling makeRoomForWrite.

Close cockroachdb#2680.
jbowens added a commit that referenced this issue May 13, 2024
@jbowens
db: avoid acquiring DB.mu during batch commit, unless rotating memtable
445236e 
Previously, DB.commitWrite would acquire the DB.mu before calling
makeRoomForWrite to ensure the current memtable had sufficient space for the
batch's contents. When DB.commitWrite is called, the caller already holds the
commit pipeline's mutex providing mutual exclusion with other committers and,
crucially, preventing the concurrent rotation of the memtable.

This commit adjusts DB.commitWrite to attempt reserving space in the current
mutable memtable before acquiring DB.mu. In the common case, if successful,
there is no need to acquire DB.mu. If unsuccessful, the committer falls back to
acuqiring DB.mu and rotating the memtable through calling makeRoomForWrite.

Close #2680.
Storage automation moved this from Backlog to Done May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-storage T-storage
Projects
Storage
  
Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

db: avoid acquiring DB.mu during batch commit, unless rotating memtable jbowens/pebble
1 participant
@jbowens