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

feat(search): Enable improved symbol parsing for large repos (when using Rockskip)#63988

Merged
mmanela merged 23 commits into
mainfrom
mmanela-splf-165-rockskip-only-uses-universalctags-and-is-not-using-scip
Jul 31, 2024
Merged

feat(search): Enable improved symbol parsing for large repos (when using Rockskip)#63988
mmanela merged 23 commits into
mainfrom
mmanela-splf-165-rockskip-only-uses-universalctags-and-is-not-using-scip

Conversation

@mmanela

@mmanela mmanela commented Jul 22, 2024

Copy link
Copy Markdown
Contributor

During an investigation, we saw that Rockskip was not using scip-ctags for symbol parsing when applicable. This means that

  1. Rockskip is getting less than optimal symbols for certain languages (like Go)
  2. Rockskip is getting no symbols for languages not in universal ctags (Magik)

This PR attempts to solve this problem but updating Rockskip to re-use the ctags parser pool logic from symbol service.

Key Changes

  • Update parser pool to be re-usable
  • Push common logic for parser type detection into the parser pool module
  • Update rockskip service config to take a parser pool
  • Update and add unit/integration tests

Questions

  • What performance impact will using this pooled parser have compared to its previous behavior of spawning a new ctags process each time?

Test plan

  • Add unit tests
  • Update integration tests
  • Manually test rockskip
  • Manually test symbolservice (in case of regression)

Comment thread docker-images/syntax-highlighter/BUILD.bazel
@keegancsmith keegancsmith self-requested a review July 23, 2024 06:22
Comment thread cmd/symbols/internal/parser/parser_pool.go Outdated
Comment thread cmd/symbols/internal/rockskip/search.go Outdated
@mmanela mmanela requested a review from stefanhengl July 23, 2024 17:34
mmanela added 2 commits July 23, 2024 13:36
…is-not-using-scip' of https://github.com/sourcegraph/sourcegraph into mmanela-splf-165-rockskip-only-uses-universalctags-and-is-not-using-scip
@mmanela mmanela marked this pull request as ready for review July 23, 2024 17:54
Comment thread cmd/symbols/internal/api/handler_test.go Outdated
Comment thread cmd/symbols/internal/parser/parser.go Outdated
Comment thread cmd/symbols/internal/parser/parser.go
Comment thread cmd/symbols/internal/parser/parser_pool.go Outdated
Comment thread cmd/symbols/internal/parser/parser_pool.go Outdated
Comment thread cmd/symbols/internal/parser/parser_pool.go Outdated
Comment thread cmd/symbols/internal/rockskip/search.go
Comment thread cmd/symbols/internal/rockskip/search.go
Comment thread cmd/symbols/internal/rockskip/search.go
Comment thread cmd/symbols/shared/setup.go Outdated
Comment thread cmd/symbols/rockskipintegration/main_test.go
@mmanela mmanela requested a review from keegancsmith July 24, 2024 19:10
Comment thread cmd/symbols/internal/parser/parser_pool.go
Comment thread cmd/symbols/internal/api/handler_test.go
Comment thread cmd/symbols/internal/rockskip/mocks_test.go Outdated
Comment thread cmd/symbols/internal/rockskip/server_test.go Outdated
@mmanela mmanela changed the title feat(search): Enable scip-ctags symbol parsing for Rockskip feat(search): Enable improved symbol parsing for large repos (when using Rockskip) Jul 31, 2024
@mmanela mmanela merged commit b2e550c into main Jul 31, 2024
@mmanela mmanela deleted the mmanela-splf-165-rockskip-only-uses-universalctags-and-is-not-using-scip branch July 31, 2024 19:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants