call computeLibraryElement on all libs, not just entry points

[jakemac53]

cc @eernstg @sigmundch

This should resolve the issues that reflectable has been seeing, as well as a few other packages.

An alternative solution to this would be to expose a way of computing individual libraries, but for all current use cases that I know of (reflectable, initialize) this would need to be done for every library anyways :(.

From some really preliminary experiments this significantly increases overall resolve time (highly dependent on app size). However, I don't think it can really be avoided.

See dart-lang/sdk#24890 for more info.

[jakemac53]

Oh, another interesting note is that the test actually passes with or without this change, although as I understand it that should not be the case. Any ideas @bwilkerson?

Edit: It appears this test is not functioning properly, which presumably means none of these tests are since it follows the same pattern (the validator function is never called). I am looking into this.

[bwilkerson]

The test looks right to me. Not sure why it wouldn't be broken.

[jakemac53]

Ok, I fixed the test so it actually runs, and it fails without the change. I am not sure why but the transformer never runs unless there is an export in the file, very bizarre. I will continue to investigate.

[jakemac53]

@bwilkerson Ok I just added an option to disable the default behavior (see 2nd commit). I also did a format on the test but pushed that as a separate commit so it wouldn't distract from other changes hopefully.

[eernstg]

@jakemac53

I mentioned in mail that it was unclear to me where computeLibraryElement would place its side-effects. Experimenting a bit with the branch on reflectable that uses _context.computeLibraryElement (and the unpatched 'code_transformers') I have seen the following:

If I replace reflectableLibrary = _resolvedLibraryOf(reflectableLibrary) by _resolvedLibraryOf(reflectableLibrary) then reflectable continues to work on an unresolved version of reflectableLibrary, and the null constant problem reappears.

Because of this, I don't understand how you can ignore the returned result from _context.computeLibraryElement(library.definingCompilationUnit.source) on line 171 of 'lib/src/resolver_impl.dart'. In the rest of 'lib/src/resolver_impl.dart' it also looks like the value return from computeLibraryElement is never ignored.

[jakemac53]

Hmm, not sure how the tests would be passing then?

[eernstg]

I suspect that the resolved LibraryElement which is returned from computeLibraryElement is also stored "somewhere" by the analyzer, and future invocations of Element.library will return the resolved library. But the concrete LibraryElement which was passed as an argument to computeLibraryElement is just as unresolved as it used to be, so it should never be used again. Do you know for sure that this will not happen?

I also suspect that @bwilkerson knows all these things by heart and I hope he'll comment on it. ;-)

[jakemac53]

Oh ok I get what you are getting at, I will add a test which I think covers this case (and will likely fail today)

[jakemac53]

@eernstg ok I added a test and updated the code to use the newly computed library elements. However, I actually noticed that the test passes with the old code as well, which is.... confusing

[jakemac53]

@bwilkerson can you take another look and lgtm or provide feedback?

[bwilkerson]

LGTM (although _performResolve would be cleaner if converted to an 'async' body).

[eernstg]

I fetched 'code_transformers' with 'resolve-all-constants' and tried it out with reflectable. It works fine, all tests passing!

However, I also completed the branch that I had been working on locally, where reflectable starts with unresolved constants (using 'analyzer' 0.27.2 and an unpatched 'code_transformers' 0.4.0). This version performs constant resolution lazily (and only if it hasn't been done earlier), just before a constant is evaluated, that is, just before evaluationResult is invoked. The interesting bit is the performance of the three approaches.

First, with the old reflectable using 'analyzer' 0.27.1 we get this result from 10 runs of transforming all 64 test cases in 'test_reflectable', using time make b:

    50.79s user 3.92s system 123% cpu 44.202 total
    47.77s user 3.64s system 125% cpu 41.067 total
    47.24s user 2.55s system 121% cpu 41.124 total
    49.47s user 3.26s system 123% cpu 42.864 total
    49.33s user 3.58s system 123% cpu 42.945 total
    53.07s user 4.63s system 123% cpu 46.777 total
    51.46s user 4.63s system 123% cpu 45.263 total
    50.29s user 3.40s system 122% cpu 43.976 total
    47.78s user 3.08s system 121% cpu 41.852 total
    52.29s user 3.76s system 124% cpu 44.966 total
    49.94s user                       43.504 total average

Running with 'analyzer' 0.27.2 and the 'code_transformers' on branch 'resolve-all-constants' (that would become the same thing as 'code_transformers' 0.4.1) we get this:

    53.83s user 3.42s system 124% cpu 46.133 total
    51.38s user 2.63s system 121% cpu 44.413 total
    56.20s user 4.25s system 124% cpu 48.515 total
    56.79s user 3.38s system 123% cpu 48.656 total
    55.80s user 3.38s system 123% cpu 47.986 total
    55.38s user 5.70s system 125% cpu 48.709 total
    57.43s user 5.26s system 124% cpu 50.411 total
    55.91s user 4.74s system 125% cpu 48.459 total
    59.32s user 4.66s system 123% cpu 51.603 total
    56.07s user 3.57s system 123% cpu 48.414 total
    55.81s user                       48.323 total average

So this is actually about 12% slower than the old 0.27.1 approach. I don't know if this was to be expected, but it seems relevant to know.

Finally, here's the result from running the branch of reflectable where I'm using 'analyzer' 0.27.2 and 'code_transformers' 0.4.0 (which would become 0.4.1 where the resolveAllLibraries would be false), we get this:

    43.03s user 2.86s system 122% cpu 37.375 total
    42.98s user 2.73s system 122% cpu 37.270 total
    42.38s user 3.21s system 124% cpu 36.638 total
    43.40s user 3.30s system 124% cpu 37.507 total
    44.67s user 3.94s system 122% cpu 39.750 total
    46.31s user 3.89s system 123% cpu 40.588 total
    43.41s user 3.26s system 122% cpu 38.025 total
    43.31s user 2.64s system 122% cpu 37.569 total
    47.08s user 3.89s system 122% cpu 41.521 total
    48.13s user 4.47s system 124% cpu 42.233 total
    44.47s user                       38.843 total average

So the old 0.27.1 approach as actually 12% slower than this, and the resolveAllLibraries approach is about 25% slower than this fine-grained/lazy approach.

Checking out why there is such a difference, the fine-grained/lazy approach manages to resolve only 53-56 libraries out of 63-74 libraries total. The typical case is 54/63, i.e., one of seven libraries are left unresolved. This makes quite a difference, apparently.

So, in the end, I think it makes sense for reflectable to use the fine-grained approach and specify resolveAllLibraries: false.

Sorry about giving rise to this change in 'code_transformers' and then not using it. Until some time yesterday I did not think that I had access to any APIs that would allow me to do it inside reflectable.

However, I think that it makes a lot of sense to have a 'code_transformers' that resolves all constants by default, and then to allow users to exploit the improved performance associated with a false resolveAllLibraries, e.g., if they just don't care about constants.

[jakemac53]

I did also see this slowdown (although it was actually worse, but I was resolving around 400 libraries in my example). I do think it makes sense to have this on by default still though, and having the option to turn it off will allow reflectable to do the more efficient thing.

You should also investigate using the new useSharedSources option for reflectable ;).

[jakemac53]

I also just published the 0.4.1 version to pub

[eernstg]

Indeed! But we actually use a global Resolvers, in which case it would not be safe (as far as I understood). It would hardly be better to use a Resolvers that is declared in apply, created afresh for each invocation (that is, for each entry point), and then useSharedSources with that, right? We saw performance go up quite nicely when we started using the global Resolvers and it seems like this kind of sharing would do at most the same thing..

[jakemac53]

If you create the Resolvers object in your constructor of your Transformer, then it should be safe. Basically, its safe as long as the Resolvers object is only ever used in the same phase (and thus, all sources are consistent).

[jakemac53]

Referring to the global Resolvers object, that helps for rebuilds, but not for initial builds in most cases. By default it only caches/reuses things per entry point. With useSharedSources it will use a shared cache of sources across all entry points.

[eernstg]

OK, I'll look into that. But which 'phases' are we talking about? How can a transformer know whether barback will call it in one phase or multiple phases? (Presumably that depends on the order of transformers in 'pubspec.yaml'?)

[jakemac53]

Yes, barback will set up phases based on your pubspec.yaml. Some transformers are also TransformerGroups which set up explicit phases of their own (for instance the Polymer transformer is really a TransformerGroup which contains the Initialize and Reflectable transformers, among others).

Basically though, a single instance of a Transformer would never end up getting used in multiple phases in any real scenarios I know of.

[eernstg]

OK, thanks!