[SES-2021] Optimise XML loading #1512
Conversation
| @@ -1558,8 +1560,7 @@ class ConversationActivityV2 : PassphraseRequiredActionBarActivity(), InputBarDe | |||
|
|
|||
| if (indexInAdapter < 0 || indexInAdapter >= adapter.itemCount) { return } | |||
| val viewHolder = binding?.conversationRecyclerView?.findViewHolderForAdapterPosition(indexInAdapter) as? ConversationAdapter.VisibleMessageViewHolder ?: return | |||
| val visibleMessageView = ViewVisibleMessageBinding.bind(viewHolder.view).visibleMessageView | |||
| visibleMessageView.playVoiceMessage() | |||
| viewHolder.view.playVoiceMessage() | |||
There was a problem hiding this comment.
Have you tested this? The audio message plays without using that existing binding that was removed?
There was a problem hiding this comment.
Yes I have tested.
The binding has been moved into the custom view itself
| private val binding by lazy { ViewVisibleMessageBinding.bind(this) } | ||
| private val binding = ViewVisibleMessageBinding.inflate(LayoutInflater.from(context), this, true) | ||
|
|
||
| private val markerContainerBinding = lazy(LazyThreadSafetyMode.NONE) { |
There was a problem hiding this comment.
Why did you have to add the LazyThreadSafetyMode.NONE to those two bindings?
There was a problem hiding this comment.
I find it wasteful to have a thread safe lazy for UI components where you only access through main thread.
app/src/main/res/layout/viewstub_visible_message_status_container.xml
Outdated
Show resolved
Hide resolved
| @Module | ||
| @InstallIn(SingletonComponent::class) | ||
| class UiModule { | ||
| @Singleton |
There was a problem hiding this comment.
I haven't used Pools before. Is that safe for it to be a singleton instantiated once? This never need to be cleared or anything?
There was a problem hiding this comment.
I think you are right. Originally I had doubt about the memory leak perspective of using a global pool, at the end of the day, ViewHolders hold Views which hold Context under the hood. I thought Google should have thought about this and do some magical thing under the hood, until just now you prompted me to have a look again: oh well, nothing magical is happening in the pool so this is def a leaking point. I'm going to remove the use of the pool as they provide marginal benefit anyway. FYI @AL-Session
There was a problem hiding this comment.
More reasons for us to move this mammoth screen into Compose where we won't have to deal with recyclerviews and viewholders and adapters...
There was a problem hiding this comment.
Have tested & everything the PR touches that previously worked still appears to work, although it's difficult to quantify any performance changes without profiling.
I did notice that we're not specifying a size of the RecyclerViewPool and I believe the default max size is 5 (although I'm not seeing this 5 value referenced in modern docs - only in the first set of deprecated docs I came across). If we think of how many single line messages we can fit on a mobile device the number is probably closer to 13 - as such, would it be worth specifying that value as 13 via setMaxRecycledViews? (see: https://developer.android.com/reference/kotlin/androidx/recyclerview/widget/RecyclerView.RecycledViewPool)
Other than that query (feel free to address as you see fit) I'm perfectly happy to approve - nice job! =D
The performance gain mainly comes from the first few enter into the conversation screen, and it's only obvious on a cheap device. I had printed out a few measurement, my Samsung used to take around 3-4 seconds to create the activity/message views and it's down to 1 second. Obviously there are still other things to do but they are much harder to do now. |
Description
This PR tries to optimize some perspective of XML View loading, namely:
View's initialization calls. (BaseActionBarActivity.getTheme()). I tested the theme changing it still works after the change.ViewPooltoConversactionActivityV2'sRecyclerView.