search

google / dagger

A fast dependency injector for Android and Java.
https://dagger.dev
Apache License 2.0
17.41k stars 2.01k forks source link

Provide official support for `dagger-reflect` #2053

Open yogurtearl opened 4 years ago

yogurtearl commented 4 years ago

When using dagger-reflect and not applying the kapt gradle plugin at all, seeing clean builds that take 54% less time for a large app build, incremental builds that take 33% less time.

Similar to how Moshi supports both code-gen and reflect versions, it would be great if the Dagger team provided official support for dagger-reflect to make sure it keeps functional parity with dagger code gen and to make dagger-reflect work with Hilt.

bcorso commented 4 years ago

Hi Michael,

When using dagger-reflect and not applying the kapt gradle plugin at all, seeing clean builds that take 54% less time for a large app build, incremental builds that take 33% less time.

Sounds like you're talking about the "full reflection" option of dagger-reflect?

The "partial reflection" option should just work (even with Hilt) by replacing dagger-compiler with dagger-reflect, right?

Do you have any numbers on how using the "partial reflection" improves build times compared to "full reflection"?

Also, what was the total build time for the stats you mentioned?

To answer your question though, we don't currently have plans to work on a reflection version of Hilt, so kapt would still need to be used for the time being. On a related note, we do have plans to switch over to KSP once that can support Dagger, which should have an improvement over kapt.

yogurtearl commented 4 years ago

Hilt support was just a side note, the main request is first party support for dagger-reflect

Chang-Eric commented 4 years ago

I think this is something we can consider, but it is a large commitment to support an essentially second implementation of Dagger, so it isn't a decision that can be made lightly. I think also complicating that is that the official advice around using it would have to be quite nuanced. For example, something like use this as you develop, maybe in some tests, but also you probably want to run your tests with regular dagger just in case there is some variation.

This is in addition to the two flavors of Dagger code generation that we already support (regular and fastInit) and it looks like there are also two modes for dagger-reflect, so this could add to potential confusion and definitely adds to maintenance difficulties.

So I think this is a good suggestion, and we will keep this issue open, but the decision will probably take some time. As @bcorso mentions above too, we may want to evaluate this against the work to be done to support KSP and see how much of a benefit this will have over that as well.

gildor commented 4 years ago

If I would choose between KSP and official dagger-reflect, I would probably choose KSP, but dagger-reflect would be a huge thing as well, now dagger-reflect doesn't support dagger-android and Hilt

vRallev commented 4 years ago

We recently replaced Dagger with a Kotlin compiler plugin in hundreds of modules. Those modules now build roughly 65% faster (assuming that Dagger would generate Kotlin with KSP). However, Kotlinc becomes significantly slower. This overhead wouldn't exist with dagger-reflect.

Another advantage of dagger-reflect would be that it works in Java modules. KSP is obviously only suitable for Kotlin-only modules.

Chang-Eric commented 4 years ago

Just to clarify, are you saying in total, including the kotlinc slowdown, everything is 65% faster vs kapt? I would expect kotlinc to slowdown because it is doing extra work and that work has to be done somewhere.

vRallev commented 4 years ago

These modules using the Dagger annotation processor build up to 66% faster now. Stub generation + Kapt are gone, javac becomes a no-op because we generate Kotlin code (the table contains 2 pure java modules, that's why the number isn't 0). Kotlinc takes more time.

  Stub generation Kapt Javac Kotlinc Sum
Dagger 12.976 40.377 8.571 10.241 72.165
Anvil 0 0 6.965 17.748 24.713

For building entire applications we measured an average saving of 16%.

yogurtearl commented 4 years ago

For example, something like use this as you develop, maybe in some tests, but also you probably want to run your tests with regular dagger just in case there is some variation.

@Chang-Eric we run ALL tests on CI with dagger-codegen and then a subset of tests on CI with dagger-reflect and of course we ship with dagger-codegen.

But there is nothing else likely to give a 33% saving on overall app build times (besides just removing dagger altogether), so dagger-reflect is a huge win for developer productivity.

realdadfish commented 4 years ago

"Modern" Dagger is basically broken in dagger-reflect due to this not yet being supported: https://github.com/JakeWharton/dagger-reflect/issues/185.

In general I'd rather go for a KSP variant here as well, because it's likely that devs hunt dagger-reflect issues that would not actually pop up in production otherwise, but since this isn't there yet, dagger-reflect or anvil are the only go-to solutions we have right now.

JakeWharton commented 4 years ago

Send a PR?

On Tue, Sep 8, 2020, at 10:17 AM, Thomas Keller wrote:

"Modern" Dagger is basically broken in dagger-reflect due to this not yet being supported: JakeWharton/dagger-reflect#185 https://github.com/JakeWharton/dagger-reflect/issues/185.

In general I'd rather go for a KSP variant here as well, because it's likely that devs hunt dagger-reflect issues that would not actually pop up in production otherwise, but since this isn't there yet, dagger-reflect or anvil are the only go-to solutions we have right now.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/2053#issuecomment-688907150, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQIEJEJZ2DZGE2MALIN2DSEY4HVANCNFSM4QCBISYA.