ResultProvider - an alternative source of analysis results.

ResultProvider - an alternative source of analysis results.

I don't like the name, so I'm open to any ideas.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Paul Berry, 2015-12-20 15:30:48]

lgtm

https://codereview.chromium.org/1539093002/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/driver.dart (right):

https://codereview.chromium.org/1539093002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/driver.dart:516: * [WorkItem] call this method when
the [result] of the [entry] is
s/call/calls/

https://codereview.chromium.org/1539093002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/driver.dart:522: * Otherwise, return `false` and the
value will be computed.
Grammar seems a little funny here.  How about: 

"Otherwise, it returns `false` to indicate that the [WorkItem] should compute
the value."

[Brian Wilkerson, 2015-12-20 19:41:57]

Sorry, but I don't understand what this is intended to be used for. Could you
provide some context?

[scheglov, 2015-12-20 19:53:14]

On 2015/12/20 19:41:57, Brian Wilkerson wrote:
> Sorry, but I don't understand what this is intended to be used for. Could you
> provide some context?


Implementations of ResultProvider will return analysis results without using
tasks, e.g. from summaries.
For example for SDK it might look like this:

class _SdkSummaryResultProvider implements ResultProvider {
  @override
  bool provideResult(InternalAnalysisContext context, CacheEntry entry,
      ResultDescriptor result) {
    AnalysisTarget target = entry.target;
    if (result == TYPE_PROVIDER) {
      if (typeProvider == null) {
        var coreLibrary = resynthesizer.getLibraryElement('dart:core');
        var asyncLibrary = resynthesizer.getLibraryElement('dart:async');
        var coreNamespace = new PublicNamespaceBuilder().build(coreLibrary);
        var asyncNamespace = new PublicNamespaceBuilder().build(asyncLibrary);
        typeProvider =
            new TypeProviderImpl.forNamespaces(coreNamespace, asyncNamespace);
      }
      entry.setValue(result, typeProvider, TargetedResult.EMPTY_LIST);
      return true;
    }
    if (target is Source && target.isInSystemLibrary) {
      if (result == LIBRARY_ELEMENT1 ||
          result == LIBRARY_ELEMENT2 ||
          result == LIBRARY_ELEMENT3 ||
          result == LIBRARY_ELEMENT4 ||
          false) {
        LibraryElement libraryElement =
            resynthesizer.getLibraryElement(target.uri.toString());
        print(libraryElement);
        entry.setValue(result, libraryElement, TargetedResult.EMPTY_LIST);
        return true;
      } else if (result == READY_LIBRARY_ELEMENT2 ||
          result == READY_LIBRARY_ELEMENT5 ||
          result == READY_LIBRARY_ELEMENT6 ||
          false) {
        entry.setValue(result, true, TargetedResult.EMPTY_LIST);
        return true;
      } else if (result == SOURCE_KIND) {
        entry.setValue(result, SourceKind.LIBRARY, TargetedResult.EMPTY_LIST);
        return true;
      } else {
        throw new UnimplementedError('$result of $target');
      }
    }
    return false;
  }
}

[Brian Wilkerson, 2015-12-21 17:01:44]

> Implementations of ResultProvider ...

In this CL it is only possible to have a single result provider, but it sounds
like you anticipate eventually having multiple. Maybe you're planning on having
a single instance that iterates over other instances.

> ... without using tasks ...

That's something of a concern to me. I don't know how long it will take to
deserialize an average sized library, but if a task needs a dozen library
elements and it takes long enough to deserialize all of them, that could hurt
latency.

I'm willing to see this committed in order to not stop progress, but we should
keep an eye on the performance impact.

LGTM

https://codereview.chromium.org/1539093002/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/driver.dart (right):

https://codereview.chromium.org/1539093002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/driver.dart:732: if (resultProvider != null &&
I'm guessing that the null check will eventually be unnecessary because at least
one provider will always exist.

[scheglov, 2015-12-21 18:25:55]

On 2015/12/21 17:01:44, Brian Wilkerson wrote:
> > Implementations of ResultProvider ...
> 
> In this CL it is only possible to have a single result provider, but it sounds
> like you anticipate eventually having multiple. Maybe you're planning on
having
> a single instance that iterates over other instances.

That's an open question.
For now I need it only for a single result provider - for SDK.
How it will be structured later - I don't know.
Maybe layered and clustered for groups of packages depending on each other,
maybe something else.


> 
> > ... without using tasks ...
> 
> That's something of a concern to me. I don't know how long it will take to
> deserialize an average sized library, but if a task needs a dozen library
> elements and it takes long enough to deserialize all of them, that could hurt
> latency.

I see what you mean.
With a result provider we pretend that we have the results it provides.
Even though it takes some time to load these results.
And we don't return from gatherInputs(), so don't run the event loop.

Maybe we could separate checking of possibility to provide a result from
summaries and actual element model construction.
Although unless we make the deserializer task-based, or somehow else
pause/resume able, it still will take one continuous time span to deserialize a
library.
But hopefully not too long time span.


> I'm willing to see this committed in order to not stop progress, but we should
> keep an eye on the performance impact.

I absolutely agree that we should keep it performant and low-latency.
The code to use result providers will be behind a flag anyway, so we will have
time to prune any performance problems.

> 
> LGTM
> 
>
https://codereview.chromium.org/1539093002/diff/1/pkg/analyzer/lib/src/task/d...
> File pkg/analyzer/lib/src/task/driver.dart (right):
> 
>
https://codereview.chromium.org/1539093002/diff/1/pkg/analyzer/lib/src/task/d...
> pkg/analyzer/lib/src/task/driver.dart:732: if (resultProvider != null &&
> I'm guessing that the null check will eventually be unnecessary because at
least
> one provider will always exist.

[scheglov, 2015-12-21 18:27:27]

Description was changed from

==========
ResultProvider - an alternative source of analysis results.

I don't like the name, so I'm open to any ideas.

R=brianwilkerson@google.com, paulberry@google.com
BUG=
==========

to

==========
ResultProvider - an alternative source of analysis results.

I don't like the name, so I'm open to any ideas.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[scheglov, 2015-12-21 18:27:29]

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