-
Notifications
You must be signed in to change notification settings - Fork 325
Reduce freeze in ProjectUpdater #7199
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
Reduce freeze in ProjectUpdater #7199
Conversation
eb1b62b
to
5f0db19
Compare
5f0db19
to
6a841fd
Compare
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.
Other than that I don't feel comfortable to comment on your changes, since I don't understand enough of the threading model here. But I had no problem importing the plugin with your changes.
base/src/com/google/idea/blaze/base/qsync/ProjectUpdaterThreadingUtils.kt
Show resolved
Hide resolved
6a841fd
to
ca80922
Compare
fb22b90
to
5600551
Compare
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 see no technical problems, but style changes would be appreciated.
} | ||
return new AbstractMap.SimpleImmutableEntry<>(module, moduleSpec); | ||
}).toList(); | ||
return new AbstractMap.SimpleImmutableEntry<>(models, modules); |
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.
Weird choice of type. Is this just a pair of values?
@@ -135,90 +142,98 @@ private void updateProjectModel(ProjectProto.Project spec, Context<?> context) { | |||
} | |||
ImmutableMap<String, Library> libMap = libMapBuilder.buildOrThrow(); | |||
|
|||
for (ProjectProto.Module moduleSpec : spec.getModulesList()) { |
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.
IMO the good old for loop is more readable in this case.
|
||
/** An object that monitors the build graph and applies the changes to the project structure. */ | ||
import static com.google.common.collect.ImmutableList.toImmutableList; |
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 the import reshuffling? Is this enforced by CI?
}, | ||
readValue -> { |
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.
We're in the middle of a method call which spans more than 100 lines. Why not extract some private methods?
5600551
to
612938f
Compare
612938f
to
d198b02
Compare
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:
https://github.com/bazelbuild/intellij/issues/7149
Description of this change