Skip to content
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

Improve usage of realm #5297

Merged
merged 4 commits into from
Feb 22, 2022
Merged

Improve usage of realm #5297

merged 4 commits into from
Feb 22, 2022

Conversation

ganfra
Copy link
Member

@ganfra ganfra commented Feb 22, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

This PR removes usage of frozen RealmResults in the timeline, as it wasn't necessary (not switch of threads)
Also uses Realm.WRITE_EXECUTOR for awaitTransaction.
Last, not really related to realm but took the opportunity to replace ReadReceipt.User by ReadReceipt.RoomMember as we are in a room.

Motivation and context

Improves performance of realm, and reduce the consumption of disk space (before compacting) and probably ram usage.

Tests

  • Open timeline
  • Close timeline
  • Check number of Realm.globalInstanceCount
  • Repeat

Tested devices

  • [ X] Physical : Pixel 3
  • Emulator
  • OS version(s): 11

@github-actions
Copy link

github-actions bot commented Feb 22, 2022

Unit Test Results

  84 files  ±0    84 suites  ±0   52s ⏱️ -6s
157 tests ±0  157 ✔️ ±0  0 💤 ±0  0 ±0 
504 runs  ±0  504 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 4cc8016. ± Comparison against base commit 116f7d0.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed=
  • [org.matrix.android.sdk.account]
    passed=
  • [org.matrix.android.sdk.internal]
    passed=
  • [org.matrix.android.sdk.ordering]
    passed=
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed=

if (bgRealm.isInTransaction) {
bgRealm.cancelTransaction()
}
return withContext(Realm.WRITE_EXECUTOR.asCoroutineDispatcher()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it worth holding onto the Realm.WRITE_EXECUTOR.asCoroutineDispatcher() instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, at least realm is not doing that neither

@@ -90,8 +90,7 @@ internal class TimelineChunk(private val chunkEntity: ChunkEntity,
private val timelineEventsChangeListener =
OrderedRealmCollectionChangeListener { results: RealmResults<TimelineEventEntity>, changeSet: OrderedCollectionChangeSet ->
Timber.v("on timeline events chunk update")
val frozenResults = results.freeze()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤞

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main fix ^ =P

.mapNotNull {
val roomMember = RoomMemberSummaryEntity.where(readReceiptsSummaryEntity.realm, roomId = it.roomId, userId = it.userId).findFirst()
?: return@mapNotNull null
ReadReceipt(roomMember.asDomain(), it.originServerTs.toLong())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any side effects from switching User to RoomMember?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User avatar can be set per room, so maybe there will be a visible effect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this should just fix some cases where user has changed avatar/name only in the room

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some questions for my own understanding but the code changes look sound! 💯

@ouchadam
Copy link
Contributor

testing locally, I didn't see much change in realm allocations unfortunately (although that wasn't the goal of this PR so it's expected)

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (static review)
ktlint is not happy, can you have a look please?

@ganfra
Copy link
Member Author

ganfra commented Feb 22, 2022

testing locally, I didn't see much change in realm allocations unfortunately (although that wasn't the goal of this PR so it's expected)

That's sad, we will have to continue digging on that point

@bmarty bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Feb 22, 2022
@bmarty bmarty merged commit 79b511b into develop Feb 22, 2022
@bmarty bmarty deleted the feature/fga/fix_bad_realm_usages branch February 22, 2022 14:21
@ganfra ganfra mentioned this pull request Feb 23, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants