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

Conversation

@efritz
Copy link
Contributor

@efritz efritz commented Aug 9, 2020

Similar to the way we preload hover text (see c165bff), we can also preload nested container names for moniker paths. This saves us from re-tracing the same AST nodes.

We piggyback on the hover text preload step and keep track of the relevant parent nodes and stash them into a map for later fast retrieval. There's still some small opportunities here for cleaner code which I'll probably tackle later (for example, we keep the entire ast path to the current node but we only look at the last three, and only look at certain node types - we can store fewer things during this traversal).

Reviewers: The logic changes are in the first two commits. The remaining commits are code style changes, a struct rename (HoverLoader to Preloader), and test updates.

@efritz efritz changed the title Efficient field monikers Make monikers for struct fields more efficient Aug 9, 2020
@efritz efritz requested review from aidaeology and garobrik August 9, 2020 21:01
@efritz efritz marked this pull request as ready for review August 9, 2020 21:02
Base automatically changed from project-contains-edge to master August 10, 2020 20:05
@garobrik
Copy link
Contributor

which commits contain the logic changes? when i pick the two oldest it shows me 29 files changed, even though the overall PR is 10 files changed

@efritz
Copy link
Contributor Author

efritz commented Aug 12, 2020

@gbrik Let me make a review order.

@efritz
Copy link
Contributor Author

efritz commented Aug 12, 2020

@gbrik

internal/indexer/moniker.go has the original logic for monikers, which uses astutil.PathEnclosingInterval to walk to the AST from the root of the file to the node to get a path from which to extract the names of typespecs and fields. This was moved into internal/indexer/preloader.go, which was renamed from HoverLoader. The file was a rename so the diff is a bit wonky, but the only relevant lines are https://github.com/sourcegraph/lsif-go/pull/77/files#diff-34daa693f818ed4b676861a3a035e975R67 and https://github.com/sourcegraph/lsif-go/pull/77/files#diff-34daa693f818ed4b676861a3a035e975R86.

The idea here is to walk each file's AST only once and construct the moniker path for every position contained in a typespec or a field. It's then a simple lookup instead of a recalculation.

All other changes are a rename of a struct.

@efritz
Copy link
Contributor Author

efritz commented Aug 12, 2020

@gbrik Here are the two commits with the logic changes:

ef56b06
4621848

Copy link
Contributor

@garobrik garobrik left a comment

Choose a reason for hiding this comment

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

LGTM

@efritz efritz merged commit b70e34f into master Aug 13, 2020
@efritz efritz deleted the efficient-field-monikers branch August 13, 2020 00:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants