-
Notifications
You must be signed in to change notification settings - Fork 126
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
Performance : improve time to open a room. #3186
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3186 +/- ##
========================================
Coverage 76.12% 76.12%
========================================
Files 1643 1643
Lines 38668 38668
Branches 7475 7475
========================================
Hits 29437 29437
Misses 5341 5341
Partials 3890 3890 ☔ View full report in Codecov by Sentry. |
...src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/TimelineItemsSubscriber.kt
Show resolved
Hide resolved
With this PR, performance seems better, but IMHO still not close to what it should be. I don't know if EX or rust-sdk is to blame. I still see ~1s+ room opening times. For contrast, WhatsApp opens rooms instantly. This is especially obvious when you compare to EXA builds from ~6 months ago where timeline loading was so fast that it would cause visual stuttering in the animations. Whilst the dropped frames were annoying, it's a lot less annoying than looking at that dreaded spinner |
Currently we are in a state where the rust sdk has no cached timeline, so it's creating one each time. It'll be back later, so should be faster again. |
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.
I'm a bit wary about the never called RustRoomFactory.destroy()
. Other than that, it seems to have improved the situation a lot!
private val mutex = Mutex() | ||
private var isDestroyed: Boolean = false | ||
|
||
private data class RustRoomObjects( |
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.
RustRoomReferences
? Naming is hard, up to you.
@@ -70,30 +88,41 @@ class RustRoomFactory( | |||
) | |||
} | |||
|
|||
suspend fun create(roomId: RoomId): MatrixRoom? = withContext(createRoomDispatcher) { | |||
var cachedPairOfRoom: Pair<RoomListItem, Room>? | |||
suspend fun destroy() { |
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.
Is this ever called? I couldn't find anything calling 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.
Oooops, it was, but I reverted some part and forgot about this one, thanks!
oldRoom.roomListItem.close() | ||
oldRoom.fullRoom.close() |
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.
Clever!
|
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.
LGTM and tested OK, thanks!
Content
Currently when getting a room through the
client.getRoom(roomId)
method, it also initialises aMatrixTimeline
and automatically add the timeline listener.This is not ok for performance as we use the method in different contexts where we don't need to listen to timeline events.
The timeline will now be subscribed only when needed. It's still hidden by the sdk, so it remains transparent for the client .
Also, rust sdk has removed his timeline cache, getting a
fullRoom
takes some time as we need to initialise the inner timeline. I've now added a smalllruCache
to keep those in memory.Last point, the app was triggering some back pagination because of some race condition when building UI items.
Motivation and context
Closes #3179
Screenshots / GIFs
Tests
Tested devices
Checklist