-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Lazy Grid Image Demo with three different implementations #5274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… functionality: CMP, native iOS, native Android - supposed to be used to compare Compose Multiplatform performance metrics with native counter-parts.
...GridImageView/nativeAndroidApp/app/src/main/java/org/jetbrains/lazygridimage/MainActivity.kt
Show resolved
Hide resolved
...GridImageView/nativeAndroidApp/app/src/main/java/org/jetbrains/lazygridimage/MainActivity.kt
Outdated
Show resolved
Hide resolved
...GridImageView/nativeAndroidApp/app/src/main/java/org/jetbrains/lazygridimage/MainActivity.kt
Outdated
Show resolved
Hide resolved
|
|
||
| val drawableResources = remember { | ||
| List(999) { index -> | ||
| val imageFilenames = context.assets.list("drawable") ?: emptyArray() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you measure the startup time?
If so, this construction is rarely used, but it can affect performance, as it can read the list from the disk.
The same is true for context.resources.getIdentifier and Res.allDrawableResources.values.
I would move reading them out of App.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you measure the startup time?
yes, of course
I would move reading them out of App
hm, initially there was an idea to have a working app for non-complete dowloaded images.
I would think how to implement the same out of the app startup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would think how to implement the same out of the app startup.
One of ideas - pregenerate a class with hardcoded resource id's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pregenerate a class with hardcoded resource id's
sounds as overkill.
I have measured impact of allDrawableResources, etc. on startup.
On iOS it is just 4 ms of 400ms Compose App startup.
On Android it is 30 ms for CMP (allDrawableResources) and 17ms for native Android impl (context.assets.list).
The real problem is native iOS impl. Checking that an image exists in a loop using UIImage(named: name) != nil(that seems to be recommended way in iOS community) adds 250ms to startup (450 ms vs 200ms with/without the check) that is huge. @ASalavei don't you know a faster way?
So we need either to drop the idea of partial images download or generate image names at compile time and read it in all implementations. Though in CMP impl we need to call allDrawableResources anyway -- as there is no other way to get a resource by string and it sounds unnatural generating a class for this task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My motivation was that app developers usually either:
- download generic images from network using a networking API
- accessing concrete images by known id (not by a string) using a resource API
- open generic images from disk using file API (assets API on Android looks similar, but
context.filesDiris used more for such case)
The first we can't do, because we can't measure it properly.
The second (the current way) probably we can do, if accessing by a string and iterating over list don't impact performance. Seems they don't impact. But there can be questions from users if we don't mention about it (it looks suspicious). One of the options - explicitly exclude it, and mention that we didn't measure it. My first suggestion was for helping excluding this measurement from the main application.
The third will be the closest to real cases, but not sure if it is worth implementing.
sounds as overkill.
Agree
if (UIImage(named: name) != nil)
It looks like we can just remove this check, because the images are always available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there can be questions from users if we don't mention about it
In the end, we'd like to show scrolling performance of lazygrid with images that are loaded from local assets. It seems like current implementation meets this requirement. Perhaps, current native Android implementation is not so idiomatic in terms of resource usages but we have no requirement to show/measure/compare resource systems in their idiomatic form.
It looks like we can just remove this check, because the images are always available?
I used partial download in the beginning for testing purposes that's is why this code appeared. For final measurements, it is safe to assume that they are all available (and assume the same for native Android app as well, I guess)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the latest version. Now only CMP version has a little performance overhead due to allDrawableResources usage (4ms on iOS, 30 ms on Android), I believe we can live with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are loaded from local assets
There is also an important detail - both CMP resources and Android resources cache decoded Bitmap in global memory. In CMP the cache is unlimited, on Android I am not sure, it can be limited by size. It behaves this way because it is designed for a case for predefined set of images, not for a case when we need to load arbitrary images from local assets.
Caching may skew the measurements.
If we want to avoid caching, contrary to my first comment, we shouldn't use a resource API everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After we rewrote it to painterResource, the caching works in both Android implementations, it is probably fine. But not sure about the native iOS.
| import androidx.activity.ComponentActivity | ||
| import androidx.activity.compose.setContent | ||
| import androidx.compose.foundation.Image | ||
| import androidx.compose.foundation.layout.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be Compose for "Android Native"? Isn't it supposed to be xml views?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it supposed to be xml views?
No, it is not. See
…d native app, fix startup overheads on other platforms.
CMP, native iOS, native Android with the same functionality
supposed to be used to compare Compose Multiplatform performance metrics with native counter-parts.
Required for CMP-7682.
Testing
Manually
Release Notes
N/A