Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

search: see if we can remove the sync.Once optimization in gitCommitResolver.OID #3746

Closed
ijt opened this issue May 1, 2019 · 2 comments
Closed
Milestone

Comments

@ijt
Copy link
Contributor

ijt commented May 1, 2019

It may no longer be necessary now that @slimsag has added a cache that serves the same purpose.

  1. Add a benchmark that exercises gitCommitResolver.OID in an approximately realistic way, ideally by issuing some search queries.
  2. Try removing gitCommitResolver.once and see if the perf is affected.
@ijt ijt added the search label May 1, 2019
@ijt ijt added this to the 3.4 milestone May 1, 2019
@slimsag
Copy link
Member

slimsag commented May 1, 2019

I mentioned this in #3685 -- this isn't the right approach.

This is not possible without a significant undertaking. In order to verify this, we would need to vet or add benchmarks for every downstream consumer of this method: https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/cmd/frontend/graphqlbackend/git_commit.go#L86:29&tab=references

That is to say, it is not just search that exercises this codepath. Just benchmarking search queries is not enough to ensure removing gitCommitResolver.once is safe.

While we could vet and benchmark all of these downstream consumers, the time would be better spent refactoring code so that Zoekt hands us the indexed revision as part of search results so that we are not doing lazy OID resolution in the first place. I'm filing an issue detailing this.

@slimsag
Copy link
Member

slimsag commented May 1, 2019

@slimsag slimsag closed this as completed May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants