-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
refactor: use @Immutable, add ImmutableWrapper
and ImmutableListWrapper
to make @Composable functions skippable
#664
Conversation
…Wrapper` to make @composable functions skippable
…Wrapper` to make @composable functions skippable
…Wrapper` to make @composable functions skippable
…Wrapper` to make @composable functions skippable
ImmutableListWrapper
to make @Composable functions skippableImmutableWrapper
and ImmutableListWrapper
to make @Composable functions skippable
core/ui/src/main/java/com/google/samples/apps/nowinandroid/core/ui/NewsResourceCard.kt
Outdated
Show resolved
Hide resolved
…e/ui/NewsResourceCard.kt
Nice 🤤 |
…ections' into hoc081098/immutable_wrapper_collections
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.
Interesting, but I'm not sure I like the increased complexity and decreased readability. Let's ask the compose expert. @simona-anomis would you be so kind and check this PR?
…oc081098/immutable_wrapper_collections
...s/src/main/java/com/google/samples/apps/nowinandroid/feature/bookmarks/BookmarksViewModel.kt
Outdated
Show resolved
Hide resolved
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.
Thank you for the contribution and effort. Unfortunately, we can't merge a large PR like this. To land a performance change like this you would need to show the changes actually improved performance and were worth the increase in code complexity. You can see an example of this here, #380
You would also need to split the PR up into smaller changes with an accompanied benchmark test for each change showing the before and after performance change please.
Thank you for your hard work. As Ben says, we can't merge it as is, but you can still create smaller PRs with documented performance improvements. |
@Immutable
to Ui state classes.ImmutableWrapper<T>
instead ofT
when T is unstable (eg. domain model) in composable functions to make them skippable (ImmutableWrapper is @JvmInline value class -> no performance overhead).ImmutableListWrapper<T>
instead ofList<T>
when T is unstable (eg. domain model) in composable functions to make them skippable (ImmutableListWrapper is @JvmInline value class -> no performance overhead).kotlinx.collections.immutable.ImmutableList<T>
/kotlinx.collections.immutable.ImmutableSet<T>
instead ofList<T>
/Set<T>
when T is stable.CURRENT
BOOKMARKS
FOR YOU
INTERESTS
TOPIC
APP
CORE UI
AFTER
There are some unskippable functions left:
NiaApp
,NiaNavRail
,NiaBottomBar