Clone SourceFactory to ensure that it does not bring a ContentAnalysis reference.

Clone SourceFactory to ensure that it does not bring a ContentAnalysis reference.

TBR

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

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

[scheglov, 2016-10-28 17:24:00]

Description was changed from

==========
Clone SourceFactory to ensure that it does not bring a ContentAnalysis
reference.

TBR

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

to

==========
Clone SourceFactory to ensure that it does not bring a ContentAnalysis
reference.

TBR

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

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

[scheglov, 2016-10-28 17:24:02]

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

[Brian Wilkerson, 2016-10-31 13:16:57]

https://codereview.chromium.org/2453773006/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/driver.dart (right):

https://codereview.chromium.org/2453773006/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/driver.dart:181: : _sourceFactory =
sourceFactory.clone() {
It seems to me that this ought to be the responsibility of the client, and that
putting this here adds a performance burden that will be paid by all clients
even if they don't need it. Granted that the cost is low, but the principle is
still the same.

[Paul Berry, 2016-10-31 18:08:59]

Regarding Brian's concern, I agree that this is a gray area.  The tradeoff as I
see it is between an API that is easier to use correctly, versus an API that
allows code to be more efficient.  To decide which way to make the tradeoff, I'm
curious to know:

(a) what does the code at the call site look like where this bug cropped up? 
(If the code at the call site is obviously dangerous-looking, I'd be more
inclined to put the burden on the caller to use the API correctly; if the code
at the call site looks fine to an untrained eye, I'd be more inclined to go with
the safer API).

(b) how expensive is it to clone the SourceFactory, and how often does the
cloning happen in a typical application?  (If the cloning is frequent and/or
expensive, I'd be more inclined to take the extra risk for the sake of
performance; if the cloning is rare and cheap, I'd be more inclined to go with
the safer API).

I don't know the answer to (a) because I don't know where the problem arose.  I
suspect the answer to (b) is that the cloning is rare and fast.  So in the
absence of more information, my preference is to go with Konstantin's change.

lgtm.

[Brian Wilkerson, 2016-10-31 18:19:35]

> (b) how expensive is it to clone the SourceFactory, and how often does the
> cloning happen in a typical application?
> 
> I suspect the answer to (b) is that the cloning is rare and fast.

I don't know the answer either, but I'm guessing that cloning is relatively
fast, but happens fairly frequently in server and never in most other clients.
It seems a shame to make other clients pay for support only needed in server (if
my guess is correct).

The other side of (a) is how frequently we need to clone the source factory in
order to be correct. If the caller can determine the one-time-out-of-1000 when
it's necessary and avoid the cost the rest of the time, then it seems worth
making this explicit.

At the very least we should document the reason for the clone.

[scheglov, 2016-10-31 18:22:43]

On 2016/10/31 18:19:35, Brian Wilkerson wrote:
> > (b) how expensive is it to clone the SourceFactory, and how often does the
> > cloning happen in a typical application?
> > 
> > I suspect the answer to (b) is that the cloning is rare and fast.
> 
> I don't know the answer either, but I'm guessing that cloning is relatively
> fast, but happens fairly frequently in server and never in most other clients.
> It seems a shame to make other clients pay for support only needed in server
(if
> my guess is correct).
> 
> The other side of (a) is how frequently we need to clone the source factory in
> order to be correct. If the caller can determine the one-time-out-of-1000 when
> it's necessary and avoid the cost the rest of the time, then it seems worth
> making this explicit.
> 
> At the very least we should document the reason for the clone.

We get SourceFactory and AnalysisOptions using the following code:

      ContextBuilder builder = createContextBuilder(folder, options);
      AnalysisContext context = builder.buildContext(folder.path);
      sourceFactory = context.sourceFactory;
      analysisOptions = context.analysisOptions;
      context.dispose();

  createContextBuilder() calls ContextBuilder.buildContext() and when it assigns
"context.sourceFactory", this SourceFactory instance becomes bound to the
AnalysisContext instance.

  I will add a comment explaining why we clone SourceFactory instances.

[Brian Wilkerson, 2016-10-31 18:28:29]

Have you considered using the pieces of ContextBuilder that create the source
factory and analysis options without actually creating an analysis context? Or
better yet, add a method like buildDriver that will build an AnalysisDriver?
(We'll probably want to change the name eventually, but that's a longer term
issue.)

[scheglov, 2016-10-31 18:36:30]

On 2016/10/31 18:28:29, Brian Wilkerson wrote:
> Have you considered using the pieces of ContextBuilder that create the source
> factory and analysis options without actually creating an analysis context? Or
> better yet, add a method like buildDriver that will build an AnalysisDriver?
> (We'll probably want to change the name eventually, but that's a longer term
> issue.)

That may be a bigger change than it seems.
AFAIK creating AnalysisOptions is done using AnalysisContext in ContextBuilder.

Adding ContextBuilder.buildDriver() seems fine, although I'm a little nervous
about making AnalysisDriver visible through ContextBuilder which some of our
clients may think of as an API.