Skip to content
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

Parallel resolve plugin entries #6972

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

DoroNahari
Copy link
Contributor

@DoroNahari DoroNahari commented Jan 27, 2020

Signed-off-by: Doron Nahari [email protected]

What it does

Our theia extension is loaded with high number of plugins causes the ide to be started but the plugins are not resolved/ready yet for a few minutes.

This pr resolve the plugins in parallel with regards to their dependencies. i.e. resolve all plugins and collect their dependencies and then resolve all the collected dependencies and so on.

Perfomance improvement:
By deploying java-pack vscode extension
Before:
perf-before
After:
perf-after

How to test

Add many plugins to Theia at once (preferred with dependencies) and see how they (and their dependencies) resolve in parallel.

For example: deploy vscode java pack: https://marketplace.visualstudio.com/items?itemName=vscjava.vscode-java-pack and check out the console logs for deployment time.

I created a pr with old code and added same logs for comparing the performance: #6982

Review checklist

Reminder for reviewers

for (const [dependency, deployableDependency] of dependencies) {
}));
for (const dependencies of dependenciesChunk) {
for (const [dependency, deployableDependency] of dependencies!) {
Copy link
Member

Choose a reason for hiding this comment

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

dependencies could be undefined and then you'll get "undefined is not iterable"

Copy link
Member

Choose a reason for hiding this comment

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

Only non-undefined values seem to be pushed inside the array, doesn't seem to be any issue here. Typing could be refined earlier maybe.

Copy link
Member

Choose a reason for hiding this comment

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

Right. So maybe change line 120 typing to be Array< Map<string, string>>

Copy link
Member

Choose a reason for hiding this comment

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

and remove ! here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@akosyakov akosyakov added the plug-in system issues related to the plug-in system label Jan 28, 2020
@akosyakov
Copy link
Member

For optimizations numbers should be provided before and after to see there it really makes the difference. Anyone should be able to follow steps in how to test and reproduce these numbers.

@akosyakov akosyakov added the performance issues related to performance label Jan 28, 2020
@DoroNahari
Copy link
Contributor Author

For optimizations numbers should be provided before and after to see there it really makes the difference. Anyone should be able to follow steps in how to test and reproduce these numbers.

Please see the updated how to test details.

@DoroNahari
Copy link
Contributor Author

@akosyakov Can it be merged? I'll be glad if it will be included in the next release (0.15.0)

}
visited.add(current);
}
queue = [];
Copy link
Member

Choose a reason for hiding this comment

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

why is it necessary? it can be reached only if length of queue is 0

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It seems to be 2x faster to deploy if extensions have dependencies.

Next time please share Node.js snapshot of the inspection session which shows that it is indeed the biggest bottleneck.

@akosyakov
Copy link
Member

@amiramw @benoitf fine to merge?

@benoitf
Copy link
Contributor

benoitf commented Jan 29, 2020

@akosyakov yes 👍

@amiramw amiramw merged commit 9a2bc81 into eclipse-theia:master Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance issues related to performance plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants