Dagger: Dagger spends a lot of time formatting code

[cypressious]

In trying to optimize my build times, I profiled a typical (warmed up) Gradle build of my Android project with YourKit and noticed that it takes a lot of time generating Dagger code. I noticed that Dagger spends especially a lot of times in the
com.google.googlejavaformat.java.Formatter.formatSource(CharSource, CharSink)
method.

I'm not sure if formatting the source code is mandatory for it to compile. If not, I would like to ask for an option to disable source code formatting.

Dagger version used: 2.2

[tbroyer]

See also https://github.com/google/dagger/issues/228#issuecomment-207996611 and @ronshapiro's answer.

[cypressious]

Oh well, I guess here's another argument for removing it.

[vanniktech]

Are there any actions taken on this one?

[ronshapiro]

The data in that screenshot shows about 1.5s of time spent formatting. That's really not a lot. If you want to speed up Dagger in general, let's focus on the parts of the code that are more expensive and run more frequently.

Feel free to reopen this if you have more data (and using the latest version of Dagger).

[JakeWharton]

A 10% speedup in annotation processing time seems like a lot. Especially since we can get it for free.

[JakeWharton]

And as you make the rest of Dagger faster, that percentage will only increase.

[ronshapiro]

8ee2c25e278c1b47058564c317416943eeebfa83 and 0fe3d92cdbccd449b070aacf3f90907d8e4d18ef (committed a week after this issue was opened) had massive (at least an order of magnitude) speedups for some projects. I'd like to know more about what we're talking about here - what's the size of the code that's being formatted that's taking 1.5s, to start - before we consider giving an option to remove this.

[cypressious]

@ronshapiro I timed an incremental build (only change is a new line) of my project with different versions of Dagger. For every test, I warmed up the gradle daemon VM by building the project 3 times. Here are the results:

Dagger 2.0

   2535ms  :blsAndroid:compileDevMultidexLollipopDebugKotlin
   3271ms  :blsAndroid:compileDevMultidexLollipopDebugJavaWithJavac

Dagger 2.5

   2561ms  :blsAndroid:compileDevMultidexLollipopDebugKotlin
   5341ms  :blsAndroid:compileDevMultidexLollipopDebugJavaWithJavac   

Dagger 2.6

   2293ms  :blsAndroid:compileDevMultidexLollipopDebugKotlin
   5038ms  :blsAndroid:compileDevMultidexLollipopDebugJavaWithJavac

Dagger HEAD-SNAPSHOT without formatting

   2532ms  :blsAndroid:compileDevMultidexLollipopDebugKotlin
   4030ms  :blsAndroid:compileDevMultidexLollipopDebugJavaWithJavac

The project is 99% Kotlin so basicly all javac is doing, is annotation processing. I included the kotlinc times so you can see the relative durations and that the test runs are comparable.

As you can see, the speed up in Dagger 2.6 was around 300ms in comparison to 2.5. Disabling formatting in the current snapshot (commit e9066f6) saves an additional second. Dispite all this, the fastes version remains 2.0.

(Incremental) Android development is already slow enough. In my opinion a time save of a whole second is a lot because it adds up quickly. For now, I'm sticking to version 2.0 as I don't use any features of the newer versions. I'd like to switch though if the performance was at least on par.

Here's the patch I applied to dagger.internal.codegen.SourceFileGenerator to disable formatting:

@@ -74,17 +71,9 @@
       final JavaFileObject sourceFile = filer.createSourceFile(
           generatedTypeName.toString(),
           Iterables.toArray(javaFile.typeSpec.originatingElements, Element.class));
-      try {
-        new Formatter().formatSource(
-            CharSource.wrap(javaFile.toString()),
-            new CharSink() {
-              @Override public Writer openStream() throws IOException {
-                return sourceFile.openWriter();
-              }
-            });
-      } catch (FormatterException e) {
-        throw new SourceFileGenerationException(
-            Optional.of(generatedTypeName), e, getElementForErrorReporting(input));
+
+      try (Writer writer = sourceFile.openWriter()) {
+        javaFile.writeTo(writer);
       }
     } catch (Exception e) {
       // if the code above threw a SFGE, use that

[JakeWharton]

Just to circle back on this, we see formatting taking approximately 40% of Dagger's processor time. I would really love to get rid of this wasted time on local developer builds where 2 seconds is a non-trivial addition to the wall clock time of a build.

[JakeWharton]

It's the two red com.google.googlejavaformat blocks which are _adjacent_ to a blue block. The blue is the file I/O which would be present regardless of formatting.

[cypressious]

Agree wholeheartedly

[ronshapiro]

We're taking a look into this, hopefully by optimizing GJF.

[bsideup]

Could you please make it configurable? We don't want to format our code, also Google's code standard is not what we follow. Pretty please.

[gk5885]

This bug tracks the performance of the formatting. For any discussion of an option to disable formatting, please file a separate feature request.

[JakeWharton]

I'm particularly interested in what pragmatic changes we can make in JavaPoet to support _some_ formatting. I believe @ronshapiro or maybe it was you Greg that suggested running GJF only on a CodeBlock for method body contents and leaving the rest to the default JavaPoet formatting.

_(Originally posted at https://github.com/google/dagger/issues/515#issuecomment-264262479)_

[gk5885]

That was me. I was talking with @cushon about this yesterday. It sounds like everybody agrees that this seems like it should improve things, but we don't really know until we make the changes to both GJF and JavaPoet and measure the effects. If I get some free time, I'll see if I can can make both changes and see what happens to some of our bigger components. If we get good results, I can send p/rs to both.

[ronshapiro]

What about field declarations? Do we declare that something like

private Provider<Map<Class<? extends AppCompatActivity>, LayoutGenerator<? extends AppCompatActivity, FragmentFactory>>>

is not something we'd care about? That's probably not the end of the world, but something that we should consider

[JakeWharton]

In theory JavaPoet could be augmented to break certain type declarations if
it surpasses 100 columns, but yeah I'm not sure what the complexity there
looks like to know if it's worth the effort.

On Thu, Dec 1, 2016 at 2:16 PM Ron Shapiro [e-mail] wrote:

What about field declarations? Do we declare that something like

private Provider, LayoutGenerator>>

is not something we'd care about? That's probably not the end of the
world, but something that we should consider

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/google/dagger/issues/368#issuecomment-264265884, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEEQ-kY5OF-gaMq6k7QQfs5k5U62Kiks5rDx0cgaJpZM4IVd6Y
.

[Dunemaster]

We have a rather larger project, which builds about two minutes with six threads with maven

About 30% percent is spent in java formatting
Speeding up formatting will result in much more comfortable experience for developers

[msridhar]

Hi, wanted to share some numbers from compiling a large module we have that uses Dagger.

Overall compile: 55s
Time spent in Error Prone checks: 23s
Time spent in dagger.shaded.auto.common.BasicAnnotationProcessor.process(Set, RoundEnvironment): 9352ms
Time spent formatting generated code in GJF: ~5s

Relevant screenshot from YourKit:

So, GJF time is around 50% of the time spent in Dagger on this module, 9% of the overall compile time, and 15% of compile time if we exclude Error Prone checks.

I modified Dagger to disable GJF formatting, and the overall time spent in BasicAnnotationProcessor.process() went down to 4885ms. So it seems having the option to disable GJF formatting can lead to a measurable reduction in compile time, at least for large modules.

I am happy to open a new pull request that is basically a rebased version of #513, adding an option to disable formatting; just let me know. Thanks!

[JakeWharton]

Can we revisit this now that JavaPoet has things like optional line breaks when column limits are hit?

What else can I add to JavaPoet to eliminate the need to run GJF? Can we disable it for now and treat any unsavory output as the bug that needs fixed?

[PaulWoitaschek]

Or for now just add a compiler flag for now that just disables any pretty printing if developers really don't care?

This issue is open for more than two years. Just sum up all the seconds of all developers waiting for their machines to format the dagger code ;)

[ronshapiro]

@JakeWharton I had experimented with the zero-width spaces and was hoping for better results, but unfortunately it results in method calls that really should be nested becoming a big block like this:

callThisMethod(Foo_Factory.callAnotherMethod(WithThisArgument_Factory.proxyInline(
    getInlineDependency()), SecondArgumentToFooFactory.newInstance(WhereAmIRightNow.newInstance(
    VeryHardToTellWhereThisIsBeingPassedTo() ...

Internal CL: http://cl/184173015 (sorry that it's out of date..., you may need to sync back if you want to build it)

It also had results that weren't as promising as I had hoped - a 5.6% decrease in generation time with this in but formatting off. That is better than nothing, but definitely still a problem.

Furthermore, it makes the Dagger code gross. We could make higher-level APIs to make it easier to specify. but I'm not convinced that it's worth it given the numbers above.

---

I haven't lost track of this issue, but I believe that it's racing forward in the wrong direction for the desired result. We have two exciting projects that we're working on to improve build times that I hope will have much better results.

1. We have an attempt to generate "ahead of time subcomponents". The idea is that currently Dagger generates every subcomponent generation in the same Java file as the root @Component. That's because we only know the full shape of the subcomponent until we encounter the root. "Ahead of time Subcomponents" is attempting to generate _as much_ of the subcomponent implementation right away as we can, with the hopes that when we begin generating the root component, there isn't much left to change+add to. Sharding compilation = less code, and hopefully simpler code for javac. For those following along, CompilerOptions.aheadOfTimeSubcomponents() is an entry point that can lead you to the progress on this so far.

2. More aggressive is an idea I've been calling "BytePoet". It takes a similar structure to JavaPoet but generates _bytecode_ directly, skipping javac entirely. The idea is still nascent, but my prototype is promising: even the simplest of classes generates in 25% of the time as the equivalent Java code. Assuming this idea comes to fruition, we'll have a toJavaPoet() method that can be used in production to get the snazy line numbers sent to your bug tracker, but development will be much faster.

My hope is that both of these (hopefully combined!) will outperform removing formatting.

[jshvarts]

FYI: This was done here: https://github.com/google/dagger/pull/513#issuecomment-427657801

[ronshapiro]

We are leaving this open so we can track improving the time it takes to format for those that want to keep it on.

[R4md4c]

@ronshapiro I tried disabling the code formatting, but unfortunately for me, the results were not satisfying, on the contrary the build time got increased.

Time taken for a clean build to a multi module project is 3m 24s with formatting disabled, to 2m 34s with formatting enabled (one minute difference here)

Perhaps because I'm using kapt? or my configuration is wrong?

Here is how I configured (applies to all modules) it

kapt {
    arguments {
        arg('dagger.formatGeneratedSource', 'disabled')
    }
}

Also I noticed that there is not much difference with the produced code, the only difference I see is that there is no line break at the closing bracelet of the function. For Example.

// With Formatting disabled
 @Override
  public void injectMembers(YanaTapTargetView instance) {
    injectStatusBarProvider(instance, statusBarProvider.get());
    injectPreferenceProvider(instance, preferenceProvider.get());
    injectMEventReporter(instance, mEventReporterProvider.get());}

// With formatting enabled
@Override
  public void injectMembers(YanaTapTargetView instance) {
    injectStatusBarProvider(instance, statusBarProvider.get());
    injectPreferenceProvider(instance, preferenceProvider.get());
    injectMEventReporter(instance, mEventReporterProvider.get());
  }

EDIT: The kapt task execution time from two build scans.

// When formatting disabled  
:app:kaptInternalStagingDebugKotlin  52.552s

// When formatting enabled
:app:kaptInternalStagingDebugKotlin 43.453s

[ronshapiro]

Can you share your secret sauce? I think we'd all want formatting to be 50% faster than not formatting!