storage: Disable timed mutex by default

[bdarnell]

These messages currently trigger very often and scare users because
they look like exceptions or crashes. We should only have them enabled
on our test clusters where we are watching for them.

---

This change is 

[a-robinson]

---

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/storage/timedmutex.go, line 97 at r1 (raw file):

	tm.mu.Lock()
	atomic.StoreInt32(&tm.isLocked, 1)
	tm.lockedAt = timeutil.Now()

Should we avoid doing this if cb is nil? I'm not sure how expensive getting the current time is (although I probably should know it...)

---

Comments from Reviewable

[a-robinson]

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

Comments from Reviewable

[bdarnell]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/storage/timedmutex.go, line 97 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Should we avoid doing this if cb is nil? I'm not sure how expensive getting the current time is (although I probably should know it...)

Good idea; I added a second commit.

Getting the current time is a system call (I saw tons of these when running cockroach under strace last week; this is probably where they were coming from). System calls are 100ns (4x more expensive than a mutex lock/unlock pair) according to https://gist.github.com/nelsnelson/3955759

---

Comments from Reviewable

[petermattis]

Getting the current time is a system call.

Eh? I'm pretty sure time.Now() does not involve a system call on Darwin or Linux but a call to the CPU timestamp counter + invocation of code provided by the kernel in a page shared with the user process. On Linux, this involves __vdso_gettimeofday.

[tamird]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[bdarnell]

Eh? I'm pretty sure time.Now() does not involve a system call on Darwin or Linux but a call to the CPU timestamp counter + invocation of code provided by the kernel in a page shared with the user process. On Linux, this involves __vdso_gettimeofday.

Ah, interesting. I called it a system call because it showed up in strace, but I guess that's not true.

[a-robinson]

Ah, interesting. I called it a system call because it showed up in strace, but I guess that's not true.

I tried benchmarking it for fun, and on my macbook I get

time.Now() = 11.0 ns/op
mutex.Lock() + mutex.Unlock() = 23.8 ns/op
int++ = 0.32 ns/op

So it's definitely not going through a system call.

[bdarnell]

on my macbook...it's definitely not going through a system call.

Note that the vsdo trick Peter mentioned is linux-specific. MacOS has its own trick to get the time without a system call, but you can't automatically generalize from one platform to the other.