db: avoid acquiring DB.mu during batch commit, unless rotating memtable

[jbowens]

db: move log bytes-in to an atomic

Move the log bytes-in metric out of the protection of db.mu and into an atomic.

db: avoid acquiring DB.mu during batch commit, unless rotating memtable

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.

[cockroach-teamcity]

This change is 

[RaduBerinde]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @aadityasondhi, @jbowens, and @sumeerbhola)

---

db.go line 943 at r2 (raw file):

	var err error
	// Grab a reference to the memtable. We don't hold DB.mu, but that's okay
	// for readers of d.mu.mem.mutable.

[nit] it's ok because we hold commitPipeline.mu, correct?

[sumeerbhola]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @aadityasondhi and @jbowens)

---

db.go line 965 at r4 (raw file):

				// again unnecessarily. We can refactor makeRoomForWrite to make
				// the assumption that the initial memtable is full, and only
				// attempt prepare on the rotated memtable.

Why not do this now? Are we trying to backport this?

---

db.go line 2444 at r4 (raw file):

	}

	force := b == nil || b.flushable != nil

Looks like we call this either with b == nil, which is a forced rotation, or when the memtable is already full, which is also a rotation. So this could be easily simplified as you mentioned earlier.

---

db.go line 2451 at r4 (raw file):

			if err != arenaskl.ErrArenaFull {
				if stalled {
					d.opts.EventListener.WriteStallEnd()

I think we should document that WriteStallBegin/WriteStallEnd are always paired in the context of a single call to makeRoomForWrite in that if it began a write stall, it will end it before returning.

I have initially confused that we had lost this call to WriteStallEnd, when lifting out the prepare call.

---

db.go line 2455 at r4 (raw file):

				return err
			}
		} else if !force {

!force is equivalent to b != nil && b.flushable == nil, so isn't this the same condition as the if-block?

[sumeerbhola]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @aadityasondhi and @jbowens)

[jbowens]

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @aadityasondhi and @sumeerbhola)

---

db.go line 965 at r4 (raw file):

Previously, sumeerbhola wrote…

Why not do this now? Are we trying to backport this?

Not trying to backport, just figured it might be a bit of a delicate refactor. I pushed an additional commit that attempts it.

---

db.go line 2444 at r4 (raw file):

Previously, sumeerbhola wrote…

Looks like we call this either with b == nil, which is a forced rotation, or when the memtable is already full, which is also a rotation. So this could be easily simplified as you mentioned earlier.

addressed in the new commit

---

db.go line 2451 at r4 (raw file):

Previously, sumeerbhola wrote…

I think we should document that WriteStallBegin/WriteStallEnd are always paired in the context of a single call to makeRoomForWrite in that if it began a write stall, it will end it before returning.

I have initially confused that we had lost this call to WriteStallEnd, when lifting out the prepare call.

addressed in the new commit

---

db.go line 2455 at r4 (raw file):

Previously, sumeerbhola wrote…

!force is equivalent to b != nil && b.flushable == nil, so isn't this the same condition as the if-block?

addressed in the new commit

[sumeerbhola]

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @aadityasondhi and @jbowens)

---

db.go line 2450 at r5 (raw file):

//     increase write pressure.
//
// maybeInduceWriteStall checks these stall conditions, and if present, wait for

nit: waits for ...

---

db.go line 2454 at r5 (raw file):

func (d *DB) maybeInduceWriteStall(b *Batch) {
	stalled := false
	// This function will call EventListener.WriteStallBegin at most once.  If

nit: It it does ...

---

db.go line 2456 at r5 (raw file):

	// This function will call EventListener.WriteStallBegin at most once.  If
	// does call it, it will EventListener.WriteStallEnd once before returning.
	for {

This is a nice simplification -- lifting the for loop out of makeRoomForWrite.

---

db.go line 2589 at r5 (raw file):

		// memtable rotation allocates a memtable sufficiently large. We also
		// held d.commit.mu for the entirety of this function, ensuring that no
		// other committers may have reserved memory in the new memtable yet.

nice

[jbowens]

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @aadityasondhi and @sumeerbhola)

---

db.go line 2450 at r5 (raw file):

Previously, sumeerbhola wrote…

nit: waits for ...

done.

---

db.go line 2454 at r5 (raw file):

Previously, sumeerbhola wrote…

nit: It it does ...

done.

[jbowens]

TFTRs!