gitserver: close packed-refs file in quickRevParseHead()

[ijt]

This is meant to help with a customer-reported issue about search performance.

Test plan: cd cmd/gitserver/server && go test -bench=BenchmarkQuickRevParseHead_packed_refs

Before this change the benchmark was failing with too many files open.

CC @beyang

[slimsag]

Put this in the end of the defer func() below instead:

• Then the reader doesn't have to think about the order in which defer is executed
• defer isn't free, so also worth not spawning two in such a frequently used code path

[ijt]

I think we'll have to disagree on that. I like this pattern that's easy to copy and paste and I'm following the normal pattern of how to use defer: for cleanup right after creating the resource.

See the benchmark below that shows defer costs about 100ns, negligible for most purposes.

[slimsag]

same with this one

[ijt]

Same disagreement. :)

[slimsag]

The trace is already named exec.rev-parse, what is the reason this log is useful?

[ijt]

I think I wanted something here to show when quickRevParseHead was being called. Anyway, in the other branch I put a separate span into quickRevParseHead. Adding a span takes about 7 microseconds according to this benchmark, so that's also nothing to worry about:

[ ~/src/github.com/sourcegraph/sourcegraph/bench ] go test -bench=NewTrace
goos: darwin
goarch: amd64
pkg: github.com/sourcegraph/sourcegraph/bench
BenchmarkNewTrace-4   	 1000000	      7239 ns/op
PASS
ok  	github.com/sourcegraph/sourcegraph/bench	7.329s

func BenchmarkNewTrace(b *testing.B) {
	ctx := context.Background()
	for i := 0; i < b.N; i++ {
		tr, _ := trace.New(ctx, "benchmark", "title")
		tr.Finish()
	}
}

[slimsag]

Suggested change

	tr.LazyPrintf("rev-parse")
	tr.LazyPrintf("HEAD fast-path")

[ijt]

All this trace stuff is cross-talk from a different branch. I didn't mean to include it here. Cleaned it up.

[slimsag]

I am unsure how performant /x/net/trace is and have a slight concern that, given we are talking about these entire operations usually taking ~16ms or so, we may indirectly harm perf with these lazy logs. Thoughts?

[ijt]

The things that are harming performance for us are system calls that take milliseconds to run.

I wrote a couple of benchmarks that show there's nothing to worry about with defer or tr.LazyPrintf:

[ ~/src/github.com/sourcegraph/sourcegraph/bench ] go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/sourcegraph/sourcegraph/bench
BenchmarkDefer-4        	20000000	       100 ns/op
BenchmarkLazyPrintf-4   	 5000000	       251 ns/op
PASS
ok  	github.com/sourcegraph/sourcegraph/bench	3.903s

// b_test.go 
package main

import (
	"context"
	"testing"

	"github.com/sourcegraph/sourcegraph/pkg/trace"
)

func BenchmarkDefer(b *testing.B) {
	for i := 0; i < b.N; i++ {
		defer func() {
			// Do nothing.
		}()
	}
}

func BenchmarkLazyPrintf(b *testing.B) {
	ctx := context.Background()
	tr, ctx := trace.New(ctx, "benchmark", "title")
	defer tr.Finish()
	for i := 0; i < b.N; i++ {
		tr.LazyPrintf("something something %d", i)
	}
}