Faster skip when annotation is not imported

[davidmorgan]

If you apply an annotation-triggered builder using source_gen to a codebase that mostly doesn't use it, every file gets analyzed to check for the annotation. Edit: correction; files with no annotations are skipped in some circumstances, see this comment.

It seems like a good idea to bail out earlier based on a faster check. For example: check if the package that contains the annotation is imported.

[davidmorgan]

Skip based on the presence of package:built_value/, benchmark modified so most files do not use the builder.

Without skip:

dart run _benchmark --generator=built_value --build-repo-path=$PWD/.. benchmark --size=2000 --mostly-no-codegen
built_value
shape,libraries,clean/ms,no changes/ms,incremental/ms
loop,2000,32434,8522,15341
forwards,2000,32051,8433,16447
backwards,2000,39872,9340,21883

With skip:

built_value
shape,libraries,clean/ms,no changes/ms,incremental/ms
loop,2000,27649,8904,11821
forwards,2000,28140,8786,11576
backwards,2000,24954,8866,9272

[rrousselGit]

By that standard, we should be able to skip files that do import the annotation but never apply it within the library.

[davidmorgan]

By that standard, we should be able to skip files that do import the annotation but never apply it within the library.

I was going to ask for your feedback :) ... to help end on a good API. See #4084 for the change I used for benchmarking.

What I currently have is something declarative in the yaml:

builders:
  built_value:
    defaults:
      options:
        build_runner:
          build_if:
            primary_input_contains: "package:built_value/"

It would be possible to do exactly the same in the builder code; if you're using source_gen then that would need a change to source_gen. But I like the idea of starting to add declarative features for builders: declarative is much more amenable to optimization :)

primary_input_contains with a single value is the simplest thing I could think of, but probably insufficient.

We could also do

• check multiple patterns, combine with AND or OR; so you could check import+annotation
• regexp matchers
• check the entire library, i.e. find all parts then match

I expect most builders can just use the single string match on the primary input, examples of more complex cases would be interesting.

[rrousselGit]

I guess the question is: What exactly are we skipping here?

Writing to a file? Resolving Elements for a library?

---

That "build_if" idea sounds like it'd break re-exports of annotations.

For example Freezed re-exports json_serializable for implementation detail. Because the generated code sometimes rely on the annotation. But it's public, so folks rely on this import to skip importing json_serializable entirely

[rrousselGit]

Also a common pattern of indirect annotation import is through variables

Folks may do:

import <annotation>
const variable = MyAnnotation(someParameters);

___

import <variable>

@variable
class Foo {}

[davidmorgan]

It's mostly about skipping resolving elements.

In the new log output, you can see the problem pretty clearly: a builder is listed as "no-op" if it runs but outputs nothing. One such case is a builder that checks for annotations but doesn't find them. You'll see these are about as slow as builders that do output something.

So if you have 1000 files but only 20 use json_serializable, you are currently paying far too much for the 980 files that don't use it.

If the heuristic is specified in build.yaml then users will be able to override it for their own cases, so it's technically possible to make it always work. I'm not sure if that will be enough (sufficiently user friendly) in practice, though.

That "build_if" idea sounds like it'd break re-exports of annotations.

Yes, as I have it now.

There's a lot of scope for doing something smarter.

For example, we could reverse it, and have a way of saying "this file triggers builders when directly imported". Then if you define your own, you have to do a bit of work to make them function. We could do that with annotations:

@triggersBuilders('freezed')
library;

const variable = MyAnnotation(someParameters);
---
import <variable> // builder is triggered

or in build.yaml.

This is a lot like the discussion we were having on how to trigger macros. Evaluating constants is not a good answer--it's too expensive. We really want something that can answer "no" much more quickly.

Open to ideas :) thanks.

[rrousselGit]

This only matters on a clean build, right?
It feels like the sort of things that would be solved naturally through better cache invalidation and incremental builds.

If an annotation is imported but never used, in theory the library should be marked as "not listening to anything besides Annotations", and would never rebuild when modifying one of the imported file.

[rrousselGit]

In particular, I think a good default would be a variant of GeneratorForAnnotation that:

• Listens to all "metadata" in the current library (to know if a variable implements a specific annotation)
• Listens recursively to objects annotated with the matching annotation
• Doesn't listen to anything else

So for most of those "noop files", they'd end-up not listening to anything besides package:meta and a few annotation from dart:core

[davidmorgan]

No, it matters just as much on incremental builds; see the last column in #4083 (comment).

The biggest improvement for incremental builds was 22s --> 9s for incremental builds by adding this heuristic.

"Listening" for changes in the way you suggest 100% makes sense, that's how we were designing the macros (v2) API. But there's no way to get to that from how build_runner and the analyzer work today: the analyzer can't do fine grained analysis/invalidation today, and build_runner isn't going to take over the job of analysis from the analyzer. What is happening in that direction is that work is being done on fine grained invalidation internally in the analyzer, then we can figure out how to best leverage that for codegen. But, that's a long arc of work.

Whereas, if we can come up with a good enough heuristic, we can get big improvements--such as better than 2x on incremental builds in the benchmark above--very soon :)

[rrousselGit]

I meant that incremental builds could have a different way to do this "skip".

I'm not talking about partial file analysis and such. I'm talking about BuildStep.reportUnusedAsset. We could have GeneratorForAnnotation natively call BuildStep.reportUnusedAsset for everything that's imported (directly or transitively) but not annotated by an annotation the generator cares about (or used by such an object)

Then, this skip you're implementing wouldn't even be reached during incremental builds, because pretty much everything they import would be flagged as "unused asset".

[rrousselGit]

A quick PoC should be doable by modifying GeneratorForAnnotation.generate to natively call reportUnusedAsset for everything imported besides a select few matching those criterias above.

The main pain point is that it's an exclude list, rather than an include list. So the logic of "exclude everything but a few specific Libraries" is quite tedious to write.

[davidmorgan]

Interesting, thanks.

reportUnusedAsset does not necessarily scale well: if you have to individually report unused assets for every build, that ends up having quadratic cost. (Inputs * number of unused deps). That could easily be overwhelming if there are thousands of files in the app. We could add a new reportUnusedEntrypoint that excludes the whole library cycle and its transitive imports, something like that might scale better. It's not clear to me whether the graph intersection algorithms you end up needing turn out to work well :)

Edit: or we could maybe arrange to do the computation/exclusion once per codebase instead of once per input, that would help :)

If you allow triggered based on consts that are propagated via variables then it's hard to tie down which files could change to trigger generation; you could have

import 'unrelated_file.dart';
@unrelatedAnnotation
class Foo {}

then go and make a change in unrelated_file.dart that makes it start importing json_annotation and makes the const value the triggering annotation. So actually you need to check on changes to unrelated_file.dart even though it has nothing to do with your generator.

Perhaps the strongest 100% correct thing we could say is, "if I output nothing because no annotation matched, I only need to rerun if files that transitively depend on the triggering annotation change, or if any file that I transitively depend on gets a new transitive dependency on the triggering annotation". But this feels too weak, because "transitive dependency on the annotation" is too broad: if you have a large library cycle in your app then probably most files transitively depend on the annotation.

That's why I think full const evaluation is too expensive, we need to somehow limit how far we look for triggering consts.