DroidKaigi / conference-app-2023

The Official Conference App for DroidKaigi 2023
Apache License 2.0
645 stars 206 forks source link

[Android] Scrolling on the sponsor screen is slower than other screens #1088

Closed koji-1009 closed 1 year ago

koji-1009 commented 1 year ago

Describe the bug When the sponsor screen is displayed and scrolled, it is slower to respond than other screens. (ex. About, Staff, Contributor)

To Reproduce Steps to reproduce the behavior:

  1. Go to 'About screen'
  2. Click on 'Sponsor'
  3. Scroll down and up
  4. See behavior

Expected behavior When the sponsor screen is displayed and scrolled, it is same to respond as other screens.

Screenshots

https://github.com/DroidKaigi/conference-app-2023/assets/17231507/5fc25bbe-a31d-4d06-84fe-64d00766685f

Enviroment

koji-1009 commented 1 year ago

I would like to try!

takahirom commented 1 year ago

This issue seems difficult! Can you do it!? I'm looking forward to your implementation!

koji-1009 commented 1 year ago

Maybe, related this issue. https://github.com/coil-kt/coil/issues/1610

h6ah4i commented 1 year ago

@koji-1009 @takahirom

Hi! I'm also very curious about this issue and looked into it, but would it be okay whether to share my finding here?

koji-1009 commented 1 year ago

@h6ah4i Of course! I am very interested in that!

koji-1009 commented 1 year ago

Only in the environment at hand, I was able to avoid this problem by using AsyncImage of coil-compose instead of Image. I am currently checking the difference between AsyncImage and rememberAsyncImagePainter.

h6ah4i commented 1 year ago

Thank you so much for the approvals!

Inspection result

First, I thought the sponsors screen has many images listed on it and they must be the bottleneck of the performance issue. So I used the profiler tool and network inspection tool to dig in what is going on here, and I noticed two things that affect performance significantly.

  1. The screen consumes too much memory (about ~ 1GiB)
  2. Each logo image size (dimension) are too large (W: 2572 [px] x H: 1322 [px])

Next, I made a small code change to confirm my hypothesis is correct. Replace all logo images with a small dummy ones, like the following. (It worked very well and the performance issue seems resolved!)

https://github.com/DroidKaigi/conference-app-2023/blob/59e43c83c5de19dd4d70e5309abcce1aff2157d4/feature/sponsors/src/main/java/io/github/droidkaigi/confsched2023/sponsors/component/SponsorItem.kt#L34

) {
    Image(
        painter = previewOverride(previewPainter = { rememberVectorPainter(image = Icons.Default.Image) }) {
-            rememberAsyncImagePainter(sponsor.logo)
+            rememberAsyncImagePainter(url = "https://avatars.githubusercontent.com/u/2552365?s=48&v=4")
        },
        contentDescription = sponsor.name,

Suggestion

This issue can be resolved by using smaller (appropriate) sized image files.

I think adding a thumbnail logo URL field to the API response is the most preferred way (if possible...).

Adding a caching layer of resized image files on app side should also resolves the issue, but it seems much difficult because the app have to run on multi-platform...

h6ah4i commented 1 year ago

Update: It looks like Compose ImageLoader already supports the maxImageSize option and it might work both Android and iOS...?

https://github.com/qdsfdhvh/compose-imageloader/blob/9832e7d849164d2bbb4b1c2d3dddabae0949ce35/image-loader/src/androidMain/kotlin/com/seiko/imageloader/component/decoder/BitmapFactoryDecoder.kt#L121

https://github.com/qdsfdhvh/compose-imageloader/blob/9832e7d849164d2bbb4b1c2d3dddabae0949ce35/image-loader/src/skiaMain/kotlin/com/seiko/imageloader/component/decoder/SkiaImageDecoder.kt#L35


Update-2: The maxImageSize option has been introduced in the latest version v1.6.5. It was just 5 days ago :tada: https://github.com/qdsfdhvh/compose-imageloader/releases/tag/1.6.5

NUmeroAndDev commented 1 year ago

It looks like the original image is preserved when converting to Painter. In the case of coil, it is resized to the display size, but it seems that compose-imageloader not resized.

koji-1009 commented 1 year ago

https://github.com/coil-kt/coil/blob/cb6eac058a2cd9757b7a9578185880d71d9e892d/coil-compose-base/src/main/java/coil/compose/AsyncImage.kt#L184

Perhaps, but the AsyncImage#updateRequest is critical point. It appears that the ConstraintsSizeResolver is giving the image request a reasonable size, a situation that preserves performance.

h6ah4i commented 1 year ago

@koji-1009

I've just created a new experimental branch to utilize the maxImageSize option. Can you test this? It looks the performance has been significantly improved!

https://github.com/h6ah4i/conference-app-2023/tree/feature/bump-compose-imageloader-v165-and-utilize-max-image-size-on-sponsors-screen


I suppose that we still need to take care more things like:

koji-1009 commented 1 year ago

@h6ah4i Thank you! I was just trying a similar implementation. At 300, there seems to be no problem with the performance of the sponsor screen. Around 1400~1500 larger, the sponsor screen seems to be affected by scrolling.

For this reason, it seems to me that it would be a good idea to limit the number to around 1024. I have looked at the other features of the app and consider the value to be without major problems.


Updating the min sdk seems to be the most issue. I looked at the differentials but could not determine how they were affected...

https://github.com/qdsfdhvh/compose-imageloader/compare/1.6.4...1.6.5

h6ah4i commented 1 year ago

I found one more problem with the Compose ImageLoader v1.6.5.

Their default implementation does not include the com.seiko.imageloader.option.Option (including maxImageSize) into cache key string. I have not confirmed it yet, but it seems like unexpected cache conflictions will occurs.

Fortunately, the cache key generation logic can be extensible because it is abstracted as a chain of com.seiko.imageloader.component.keyer.Keyer instances. I'm currently looking for a place where is the most relevant to inject our custom Keyer implementation.

koji-1009 commented 1 year ago

It reads as if the scale is not used when downloading the image, but the maxImageSize is used when creating the bitmap. Therefore, it seems that maxImageSize does not need to be included in the key when saving to disk.

h6ah4i commented 1 year ago

I think disk cache is okay, but the memory cache has a problem. It just returns a scaled Bitmap instance for an original source URL without considering the maxImageSize option.

h6ah4i commented 1 year ago

Wait, do we only have to take care about Android? The iOS version seems have different implementation for UI including image loader mechanism. I was trying to find an alternative to CompositionLocalProvider (ref. Use custom ImageLoader) on iOS, but it seems I'm doing something wrong.

I'm not sure how iOS version of this app works, but I thought Coil has been replaced with Compose ImageLoader in order to support iOS... (#258)

https://github.com/DroidKaigi/conference-app-2023/blob/59e43c83c5de19dd4d70e5309abcce1aff2157d4/app-ios/Modules/Sources/Sponsor/SponsorGridView.swift#L41

koji-1009 commented 1 year ago

@h6ah4i Thank you for your research! I understand now.

Would memory cache be a problem in cases where the same image is retrieved with different maxImageSize? In this case, if maxImageSize is constant, it seems to be avoidable. (Therefore, it may be OK to simply specify maxImageSize as a fixed value.)


https://github.com/DroidKaigi/conference-app-2023#module-structure

We've added experimental support for Compose Multiplatform on certain screens, making the features accessible from the iOS app module as well."

It seems that sponsor screen is not a shared target module? If it is ok to split it, that is looks good to me that replacing it with coil would be the best.

Hi @takahirom, which option should I choose?

h6ah4i commented 1 year ago

Would memory cache be a problem in cases where the same image is retrieved with different maxImageSize? In this case, if maxImageSize is constant, it seems to be avoidable.

True. That works for the simplest workaround for current situation :wink:

Also, I have created a minimal sample project to reproduce the cache confliction issue. I'm going to feedback it to the author of Compose ImageLoader project.

takahirom commented 1 year ago

I'm going to feedback it to the author of Compose ImageLoader project

I've mentioned the author but the bug report is better. So I removed the comment 🙇

takahirom commented 1 year ago

@koji-1009 I think it is good to use Coil for sponsor screen 👍

koji-1009 commented 1 year ago

@takahirom Thank you very much! I will recreate PR!

h6ah4i commented 1 year ago

FYI: I have reported the Compose ImageLoader's memory cache issue here; https://github.com/qdsfdhvh/compose-imageloader/issues/279

qdsfdhvh commented 1 year ago

Thanks for the report, I will fix it tomorrow, i'm going to io connect today.