Injection on Android is slow because of getCanonicalName()

[ctsimmonds]

I'm converting an existing Android project from using RoboGuice to Dagger, and to my surprise injection of a fragment is slower with Dagger by about 5%. From method tracing, it looks like the culprit is that the Class.getCanonicalName() method can be really slow, particularly when called on inner classes.

For one injection that I profiled, AndroidSupportInjection.inject() takes 1.074s of CPU time as measured in the profiler. Of that, the two calls to getCanonicalName() for the logger call take 67ms and 164ms respectively. Then the call to DispatchingAndroidInjector.inject() takes 696ms, all of which is spent inside maybeInject(). That method spends 483ms on another call to getCanonicalName(), and then finally 209ms for the generated inject() method for my fragment's subcomponent.

This means that 66.5% of the total time for the injection was spent inside calls to getCanonicalName(). The last instance is particularly annoying, because it's only used as a parameter to checkNotNull() and the resulting string value is only used if the injection fails and is a complete waste of time on success path.

There are a couple of ways that this time could be improved:

• Issue Turn off Logcat during Android Fragment injection #790 has already been raised to remove the logger statement, which would remove the first two calls to getCanonicalName().
• Calls to checkNotNull() should only evaluate getCanonicalName() on Class objects if the reference being checked was actually null. All of the calls to it that pass foo.getClass().getCanonicalName() would just pass the class instance instead, and getCanonicalName() would only be called before throwing NullPointerException.

[GeorgePetri]

Any progress on this? It seems to be present still on 2.12.

[netdpb]

Would replacing getCanonicalName() with getName() still be a problem? Or better yet, just use the class object.

[JakeWharton]

Using a normal if-based null check such that the exception message is only constructed inside the conditional seems like the best option to me. Then we don't waste time on string building for an uncommon case.

…

On Fri, Oct 13, 2017 at 11:51 AM David P. Baker ***@***.***> wrote: Would replacing getCanonicalName() with getName() still be a problem? Or better yet, just use the class object. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#819 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEaC25a1SmuETkAHzmEcN-pRn9PSFks5sr4cSgaJpZM4OjBdM> .

[ronshapiro]

Sorry for not doing this yet - this is totally reasonable. Do we know why calling this method is so slow though? It's very surprising to me. Should we be discouraging calling it in general?

[ronshapiro]

*discouraging it beyond Dagger

[bejibx]

Could it be because Android uses .dex format for compiled classes?