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.

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

VS Code: fix repo name resolution cache #5978

Merged
merged 4 commits into from
Oct 23, 2024
Merged

Conversation

valerybugakov
Copy link
Member

  • Ensures the repo name resolution is cached and we don't make unexpected API calls for already resolved remote URLs.
  • This fix is critical for enterprise clients because the regression significantly increases the repo-name resolution latency, which is used by every enterprise client with Cody Context filters and is on the critical path for every autocomplete request. In my local tests, it adds ~500-600ms to every repo name resolution call.
  • Slack thread with more technical details.
  • Closes https://linear.app/sourcegraph/issue/CODY-4139/investigate-repo-name-resolution-latency-increase

Test plan

CI with updated unit tests.

Changelog

  • Context Filters: fixed repo name resolution cache.

@valerybugakov valerybugakov self-assigned this Oct 23, 2024
expect(getRepoNameGraphQLMock).toBeCalledTimes(1)
})

it('reuses cached API responses that are needed to resolve enterprise repo names', async () => {
Copy link
Member Author

@valerybugakov valerybugakov Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new test fails if shouldCountRefs is set to true in getRepoNameCached.

Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work although the subtlety of this scares me about observables in general.

Is this covered by tests that changing auth will re-query for repo IDs?

You mentioned an LRU, but that is not here... Any concerns about this being a source of leaks?

@valerybugakov
Copy link
Member Author

valerybugakov commented Oct 23, 2024

Good work although the subtlety of this scares me about observables in general.

Same! It's easy to make this mistake. Similar to concerns about React's useMemo sensitivity but even subtler.

Is this covered by tests that changing auth will re-query for repo IDs?

I don't know if we have this type of test for all the features where reactivity was introduced. We should start adding more of those. I tested this case manually with a debugger for now.

You mentioned an LRU, but that is not here... Any concerns about this being a source of leaks?

The LRU cache was added to vscode/src/repository/repo-name-resolver.ts to replace regular objects. This change should keep those caches from growing forever.

@valerybugakov valerybugakov merged commit 252b40d into main Oct 23, 2024
20 checks passed
@valerybugakov valerybugakov deleted the vb/fix-repo-name-cache branch October 23, 2024 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants