google / iosched

The Google I/O Android App
Apache License 2.0
21.77k stars 6.2k forks source link

Implement App Startup #376

Closed nuhkoca closed 3 years ago

nuhkoca commented 3 years ago

This PR adds yet another Jetpack library called App Startup in order to boost startup time of the application. Here is a comparison chart for better understanding.

Screenshot 2021-06-01 at 22 59 38

Looks like there is a significant improvement at startup. If you want to test yourself, execute below command in your terminal:

Please only take care of Total Time row.

adb shell am start-activity -W -n com.google.samples.apps.iosched/.ui.LauncherActivity

nuhkoca commented 3 years ago

cc @JoseAlcerreca

JoseAlcerreca commented 3 years ago

This PR should be used as an example of how to PR.

tikurahul commented 3 years ago

Firstly, thanks for this wonderful PR. However, i am actually very surprised its making such a big difference.

That is because, we are going from code that was running inside an Application.onCreate(), to one that's running in the provider. There is about a 1-2 ms overhead of a ContentProvider. The lazy initialization of Timber should not make a lot of difference, because we end up calling it inside Application.onCreate().

Fortunately for us, there is a really good way to tell if this change is making a difference that is a net positive. Can you add a Startup macrobenchmark, similar to the one we have here.

There is also a lot of additional documentation here.

nuhkoca commented 3 years ago

Hey @tikurahul thanks for reviewing the PR. Sure thing I will implement Startup macrobenchmark, as well but I am not that familiar with it. Is it enough to just copy the same thing from the repository you nentioned?

tikurahul commented 3 years ago

Yes, pretty much. Change the intent to launch the package in question.

nuhkoca commented 3 years ago

Just wanted to update you. I was tied up with internal thing over the last week and couldn't find that time to work on this. However, I will update the PR very soon. Sorry for the delay.

nuhkoca commented 3 years ago

@tikurahul Sorry again for the belated update. I have implemented the Startup Macrobenchmark and here are the results. Looking forward to your feedback.

I can't upload HTML files directly but you can find attached. benchmark-results.zip

Without AppStartup

Screenshot 2021-06-09 at 22 55 45

With AppStartup

Screenshot 2021-06-09 at 22 55 31
tikurahul commented 3 years ago

It does seem to make a big difference. 😄

LGTM !

nuhkoca commented 3 years ago

cc @JoseAlcerreca I think this can be merged, right?

nuhkoca commented 3 years ago

@JoseAlcerreca can you help me for UI tests? After moving AnalyticsHelper out of application and inject it in the Initializer class, UI tests have start to fail and I don't know how to get it to working with Hilt.

manuelvicnt commented 3 years ago

Hi @nuhkoca ! The problem with the tests are due to EntryPoints being used. In tests, Hilt's SingletonComponent is scoped to the lifetime of the test case instead of the lifetime of the Application.

To get this fixed, you need to use @EarlyEntryPoints instead of @EntryPoints in the different initializers.

More info here: https://dagger.dev/hilt/early-entry-point.html

Let me know if you need more help :D thank you!

nuhkoca commented 3 years ago

@manuelvicnt Thanks a lot for your review. I did the same what you and documentation mentioned but tests still fail. So I need your help here :D Shall I push changes?

manuelvicnt commented 3 years ago

Let me take a look tomorrow. No need to push your changes with EarlyEntryPoint though :D

manuelvicnt commented 3 years ago

Hi @nuhkoca ! Tried locally and yes, the tests keep failing but for a different reason now. EarlyEntryPoints fixes the problem when accessing the graph in tests.

As initializers run automatically, by the time AnalyticsHelperInitializer runs and tries to access the context, there's no context because the test hasn't even started yet. And that throws the following runtime exception.

androidx.startup.StartupException: java.lang.IllegalStateException: No instrumentation registered! Must run under a registering instrumentation.

I thought about removing the automatic initialization for tests and then activating them manually in MainTestApplication, but I couldn't get it to work. Following the App Startup documentation, I added a new AndroidManifest.xml file in the androidTest folder and added:

<application>
    <provider
        android:name="androidx.startup.InitializationProvider"
        android:authorities="${applicationId}.androidx-startup"
        android:exported="false"
        tools:node="remove"/>
</application>

But that does NOT work either! Because ${applicationId} = com.google.samples.apps.iosched.test and the authority in the main app is com.google.samples.apps.iosched. And if you try to hardcode com.google.samples.apps.iosched in there doesn't work as the provider is not the same. It gives a The application could not be installed: INSTALL_FAILED_CONFLICTING_PROVIDER compilation error.

@tikurahul can you help here? Not wanting to run initializers during integration tests seems to be a common use case the community can face.

nuhkoca commented 3 years ago

@manuelvicnt Thank you for taking valuable time on this, those are really great findings and should give insights to developers in tests! I also played around a bit with Startup but unfortunately kept getting errors. Hope @tikurahul will rescue us 🤞

tikurahul commented 3 years ago

Not wanting to run Initializers for instrumentation tests is actually not that common.

I am afraid the only way to actually do this, is to actually use build variants and have a separate AndroidManifest.xml for the APK under test.

https://developer.android.com/studio/build/build-variants

manuelvicnt commented 3 years ago

Not wanting to run Initializers for instrumentation tests is actually not that common.

Well, it seems like every initializer that gets injected by Hilt needs to be deferred until the Application is created

nuhkoca commented 3 years ago

Sorry for the belated response.

I am afraid the only way to actually do this, is to actually use build variants and have a separate AndroidManifest.xml for the APK under test.

Should I create a separate AndroidManifest.xml? But I am confused how this will fix the issue 🤔

tikurahul commented 3 years ago

This change (https://github.com/google/iosched/pull/379) addresses your issues.