-
Notifications
You must be signed in to change notification settings - Fork 3
[performance] Import the large project #17
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
Conversation
e42b1ae to
9ae8b3d
Compare
6d3a529 to
87cc953
Compare
1c87a87 to
f4f63c7
Compare
|
The PR includes the folowing changes:
cc @guw |
guw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR mixes programming changes with performance fixes. It needs to be separated. So the performance improvements can be inspected separate from job/thread/scheduling changes.
The use of jobs framework is preferred. Thus, any removal of this needs really good data on why this is needed.
| } finally { | ||
| try { | ||
| refreshResources(resourcesToRefresh, monitor); | ||
| //refreshResources(resourcesToRefresh, monitor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to take a look at #16 , issue 4.
|
|
||
| // loads can be potentially expensive; we synchronize on the location | ||
| var location = getLocation(); | ||
| var openJob = new BazelElementOpenJob<>(location != null ? location : IPath.ROOT, this, infoCache); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eliminating the job removes transparency and visibility into the package. The job framework provides valuable insights in the Eclipse UI. I would rather like to see JDTLS improve here.
The second important thing provided by jobs framework is synchronization using ISchedulingRule. It is important that at most ONE Bazel command is executed per workspace in general. The Bazel client prevents concurrent executions usually, which can lead to unexpected pauses/waits when multiple Bazel commands are executed by different threads. The job framework (again) helps visualizing this in the UI.
| * @return | ||
| */ | ||
| public abstract <I extends BazelElementInfo> I putOrGetCached(BazelElement<I, ?> bazelElement, | ||
| Supplier<I> supplier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of supplier adds unwanted complexity. I experimented with suppliers here in the past. However, it was discarded because we do not want to allow computation by direction. We rather like to be in control of when Bazel commands are executed and how.
|
|
||
| private volatile Map<String, BazelRuleAttributes> externalRepositoryRuleByName; | ||
|
|
||
| private volatile Map<BazelLabel, IProject> projectByLabel = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we start caching these here we need some logic in the manager to invalidate when users open/close projects.
| .collect(toMap(BazelRuleAttributes::getName, Function.identity())); // index by the "name" attribute | ||
| } | ||
|
|
||
| public void put(BazelLabel label, IProject project) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A public put is problematic. What is the reason for this?
| // during synchronization resource changes may occur; however, they are triggered by the synchronization activities | ||
| // therefore we suspend cache invalidation of the model due to resource changes | ||
| workspace.getModelManager().getResourceChangeProcessor().suspendInvalidationFor(workspace); | ||
| workspace.getModelManager().getResourceChangeProcessor().suspendInvalidationFor(workspace.getModel()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is. I have created #44
| public Map<BazelPackage, Map<String, Target>> queryForTargetsWithDependencies(BazelWorkspace bazelWorkspace, | ||
| Collection<BazelPackage> bazelPackages, BazelElementCommandExecutor bazelElementCommandExecutor) | ||
| throws CoreException { | ||
| // bazel query 'kind(rule, deps(//foo:all + //bar:all))"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an interesting optimization for removing bazel query calls. Please submit as separate PR/commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created #43
|
@guw I have created separate PRs: |
Fixes #16
Steps to test:
to .eclipse/.bazelproject
-Xmx10G -Xms4g -Declipse.bazel.model.cache.expireAfterAccessSeconds=86400 -Djdk.xml.elementAttributeLimit=0to Eclipse