search

bazelbuild / intellij

IntelliJ plugin for Bazel projects
https://ij.bazel.build/
Apache License 2.0
760 stars 303 forks source link

Improve performance when resolving the workspace root #6530

Closed ivyspirit closed 2 months ago

ivyspirit commented 3 months ago

Checklist

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See the Contributions section in the README for more details.

Discussion thread for this change

Issue number: 5719

Description of this change

We have pretty big bazel project, and we are using the rule_jvm_external pinned feature, which download all the external dependencies to the {baze-base}/external dir. The external folder could grow very big. One example the smallest cache folder for us:

% echo "Total directories:" $(find {root}/.cache/bazel/arm64/96fb5a4ccfb8aa9cafd25443b98fa7e6/external -type d | wc -l) && du -sh {root}.cache/bazel/arm64/96fb5a4ccfb8aa9cafd25443b98fa7e6/external

Total directories: 54733
5.8G    {root}/.cache/bazel/arm64/96fb5a4ccfb8aa9cafd25443b98fa7e6/external

The original logic when resolving the dependency label, it loops through the entire {baze-base}/external dir, and check if the file is dir, then create a map for with the workspaceName as key and its dir path as value. Which when the external is really big the IO operation could hang there for a long time cause the IDE freeze.

This change use the {baze-base}/external/{workspaceName} to construct the workspace root dir. And if it existed then construct the WorkspaceRoot and return.

ivyspirit commented 3 months ago

Fixing the test

ivyspirit commented 3 months ago

The ExternalWorkspaceReferenceTest test failed, bc i checkfile.exist()here and here before i return the workspaceRoot. However in the test TestFileSystem creates the PsiFile does not seem to create file in the test dir? Any suggestion? Once I removed the file exist check the tests all passes.

ivyspirit commented 3 months ago

The ExternalWorkspaceReferenceTest test failed, bc i checkfile.exist()here and here before i return the workspaceRoot. However in the test TestFileSystem creates the PsiFile does not seem to create file in the test dir? Any suggestion? Once I removed the file exist check the tests all passes.

@sgowroji do you have any suggestion on this? in the test it create a VirtualFile by TempFileSystem. Which the file does not exist. But in my code I need to check the file exist before return. I checked the code base didn't see an example in the test to handle this case. Any suggestion would be appreciated

ivyspirit commented 3 months ago

@mai93 i have fixed the tests. All of them are due to the mock set up. Please take a look. Thanks!

mai93 commented 3 months ago

LGTM from me, @tpasternak if you have time can you review this?

tpasternak commented 3 months ago

yep, I'll try it later today

ilisc2 commented 2 months ago

Also I found this bug:

  1. Import https://github.com/bazelbuild/bazel
  2. Open the top-level BUILD file

image

Ok, this probably happens, because when you run sync, the blazeProjectData entry might be null so the null is written to the cache

https://github.com/bazelbuild/intellij/blob/eda0807688780d89bf934ec8f0368791b584e7a7/base/src/com/google/idea/blaze/base/sync/workspace/WorkspaceHelper.java#L200-L203

The local map cache should help on this. although before the blazeProjectData finish init the workspace root will not work. But when it query again once it is done initialization it should work. Wonder how did the old implementation work with this case? seems there would be NPEs. And once the map is cached to the SyncCache it will not be modified, so it might not recover.