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

[elastic] Cache the results of 'invokeGo()' call #76

Merged

Conversation

movie-travel-code
Copy link

'invokeGo' is used to get the import outline of the specified folder(package),
so there are duplications that will slow down the go langserver.

Since go langserver serves several repos at the same time, use LRU cache
in case of blow up the memory. 'determineRootDirsCached' serves single view,
i.e. single workspace folder, 'golistDriverLRUCached' serves single
subfolder, i.e. package. Given that we will skip 'vendor' folder, for now,
the number of the repos are simultaneously indexing is very low, that's
why set the 'determineRootDirsCached' entry number to 16.

Future plan:

  1. skip the 'vendor' file
  2. download the deps during the initialize request
  3. eliminate the 'invokeGo' call
  4. find a better approach to handle the 'vendor' folder

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

var (
goListLRUCache *lru.Cache
createGoListLRUCache sync.Once
goListLRUEntries = 32
Copy link

Choose a reason for hiding this comment

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

isn't 32 too smal?

I would imaging a cache at least 4MB of size in total

Copy link
Author

Choose a reason for hiding this comment

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

It's difficult to give a perfect cache size. IIUC kibana will send the full request in the order of folders, that means we won't touch the folder again in a short period of time. goListDriver() has the same result for the same folder, like A\a.go, A\b.go, A\c.go, A\d.go, once we cache the go list result of A\a.go, we can reuse the result for three remains go files. And we won't handle the same folder again in the same indexing process. So holding the go list results in the memory for a long time is not the best choice.

I use TiDB which has 820 .go file and 96 subfolders to test the which number is the best candidate. Setting 32 has the shortest time and memory usage is also within a reasonable range. I will improve the entry numbers in case we index many repos simultaneously.

Copy link
Author

Choose a reason for hiding this comment

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

Test more repos to get the best cache size.

@AppVeyorBot
Copy link

'invokeGo' is used to get the import outline of the specified folder(package),
so there are duplications that will slow down the go langserver.

Since go langserver serves several repos at the same time, use LRU cache
in case of blow up the memory. 'determineRootDirsCached' serves single view,
i.e. single workspace folder, 'golistDriverLRUCached' serves single
subfolder, i.e. package. Given that we will skip 'vendor' folder, for now,
the number of the repos are simultaneously indexing is very low, that's
why set the 'determineRootDirsCached' entry number to 16.

Future plan:

skip the 'vendor' file
download the deps during the initialize request
eliminate the 'invokeGo' call
find a better approach to handle the 'vendor' folder
@movie-travel-code
Copy link
Author

I choose 7 repos to test the cache size, these repos contain 592 go list results.

I index these repo simultaneously, the rough results are as follows.

cache size memory
512 2.60GB
4096 2.70GB

80 go list results occupy 100MB memory. Given that 512 is enough to cope with most situations, set the cache size to 512 temporarily.

@AppVeyorBot
Copy link

@movie-travel-code movie-travel-code merged commit 2702523 into elastic:master Aug 20, 2019
movie-travel-code pushed a commit that referenced this pull request Aug 20, 2019
'invokeGo' is used to get the import outline of the specified folder(package),
so there are duplications that will slow down the go langserver.

Since go langserver serves several repos at the same time, use LRU cache
in case of blow up the memory. 'determineRootDirsCached' serves single view,
i.e. single workspace folder, 'golistDriverLRUCached' serves single
subfolder, i.e. package. Given that we will skip 'vendor' folder, for now,
the number of the repos are simultaneously indexing is very low, that's
why set the 'determineRootDirsCached' entry number to 16.

Future plan:

skip the 'vendor' file
download the deps during the initialize request
eliminate the 'invokeGo' call
find a better approach to handle the 'vendor' folder
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.

4 participants