Add PluginWatcher to watch for new plugins that need to be run

Add PluginWatcher to watch for new plugins that need to be run

R=scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/8afab37d554e25fb0f7d8867603a7f75cc915090

[Brian Wilkerson, 2017-03-15 16:36:58]

brianwilkerson@google.com changed reviewers:
+ scheglov@google.com

[scheglov, 2017-03-15 17:41:34]

LGTM, but I think we need to improve performance.

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/plugin/plugin_watcher.dart (right):

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/plugin/plugin_watcher.dart:56: List<String>
addedPluginPaths = _checkPluginsFor(driver);
It seems very wasteful to me to perform this cycle-over-all-the-known-files and
look-into-every-folder for every new analysis result. I think we need to perform
this much less often, maybe schedule the 5-seconds timer after a new result. So,
we check only once after a burst of new results.

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/plugin/plugin_watcher.dart:102: for (FileState state
in driver.fsState.knownFiles) {
If the set of files is the same as it was the last time, we don't need to do
anything.

And we don't need to do anything for any file we have seen already.

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/plugin/plugin_watcher.dart:110:
addedPluginPaths.add(_locator.findPlugin(packageRoot));
Can findPlugin() return null?

Also, could we cache the fact that there is, or there is no plugin in
packageRoot? Maybe add this cache into PluginLocator. In other parts of Analysis
Driver and SourceFactory we use this approach - cache information about paths
and URIs. Unless we are told that it is changed, keep it cached. As it is now,
we are never told, and caches die with restart.

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/plugin/plugin_watcher.dart:138: ContextRoot
contextRoot;
Do these fields change over the life of the instance?

[Brian Wilkerson, 2017-03-15 18:25:46]

I'll follow-up on performance concerns in another CL.

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/plugin/plugin_watcher.dart (right):

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/plugin/plugin_watcher.dart:56: List<String>
addedPluginPaths = _checkPluginsFor(driver);
I agree that this is wasteful.

I'm not convinced that we want to wait for a fixed amount of time, but that
might be the best option.

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/plugin/plugin_watcher.dart:102: for (FileState state
in driver.fsState.knownFiles) {
> If the set of files is the same as it was the last time, we don't need to do
> anything.

True, and I can cache the previous list in order to detect this, but I have some
concerns about caching and comparing long lists.

My guess is that the list of known files is usually monotonically increasing in
size, and it would be nice if we could use the length of the list to decide
whether we need to re-examine everything. The only time it would decrease in
size is if some files are either deleted or excluded from analysis. Is there any
way to detect when a list of the same size might be different?

> And we don't need to do anything for any file we have seen already.

True. That's why I have the list of package roots, so we won't do anything if
the file is in a package we've already looked at.

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/plugin/plugin_watcher.dart:110:
addedPluginPaths.add(_locator.findPlugin(packageRoot));
> Can findPlugin() return null?

Yes. Good catch! I'll fix it before committing.

> Also, could we cache the fact that there is, or there is no plugin in
> packageRoot? Maybe add this cache into PluginLocator.

Yes. I was planning on doing that, but hadn't gotten there yet.

https://codereview.chromium.org/2750483006/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/plugin/plugin_watcher.dart:138: ContextRoot
contextRoot;
> Do these fields change over the life of the instance?

The context root doesn't. The list of package roots doesn't either, but the
content of the list does.

I assume you're suggesting that they should be final, which I'll do.

[Brian Wilkerson, 2017-03-15 18:26:18]

Description was changed from

==========
Add PluginWatcher to watch for new plugins that need to be run
==========

to

==========
Add PluginWatcher to watch for new plugins that need to be run

R=scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/8afab37d554e25fb0f7d8867603a7f75cc915090
==========

[Brian Wilkerson, 2017-03-15 18:26:19]

Committed patchset #1 (id:1) manually as
8afab37d554e25fb0f7d8867603a7f75cc915090 (presubmit successful).