Fix slow paged room list regression #6233

BillCarsonFr · 2022-06-03T12:27:12Z

Type of change

Bugfix

Content

A new parameter was added to resolve the list of parent summaries when loading the rooms paged list.
This param is affecting performances a lot when you have some decent space hierarchy. And it was called unproperly when displaying the regular room list (should only be needed for search/filter rooms).

Also the new added flattenParents field (that was slowing the queries) was never used. Instead the space parent was used. I think improperly because it only reflect the presence of a m.space.parentrelation, that is not used to decide hierarchy (m.space.child) are used for that.

@ericdecanini can you confirm?

See #3830 regarding recent degradation of performances.

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

github-actions · 2022-06-03T12:34:37Z

Unit Test Results

148 files ±0 148 suites ±0 2m 12s ⏱️ -32s
237 tests ±0 237 ✔️ ±0 0 💤 ±0 0 ❌ ±0
790 runs ±0 790 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 89e528d. ± Comparison against base commit 0ef67b6.

♻️ This comment has been updated with latest results.

ericdecanini

I think the doc change is very much needed. There's a similar method in RoomService called getPagedRoomSummariesLive that feels like the default method used while getFilteredPagedRoomSummariesLive sounds like it's only used in FilteredRoomsActivity. This issue highlights that this isn't the case, and it would be good to make that clearer in the code through these java docs

ericdecanini · 2022-06-03T12:35:44Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/RoomService.kt

     */
    fun getFilteredPagedRoomSummariesLive(
            queryParams: RoomSummaryQueryParams,
            pagedListConfig: PagedList.Config = defaultPagedListConfig,
-            sortOrder: RoomSortOrder = RoomSortOrder.ACTIVITY
+            sortOrder: RoomSortOrder = RoomSortOrder.ACTIVITY,
+            getFlattenParents: Boolean = false,


Suggested change

getFlattenParents: Boolean = false,

getFlattenParents: Boolean = false, // Heavily impacts performance when true. Only use when space parent is absolutely required

Good point, but please add this mention in the Kdoc and not here, or it will not be exported.

ericdecanini · 2022-06-03T12:39:10Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/RoomService.kt

     * TODO Doc.
+     * @param getFlattenParents When true, the list of known parent and grand parent summaries will be resolved.
+     * This can have significant impact on performance, better be used only on manageable list (filtered by displayName, ..).


Feels weird to have a partial java doc covering only one parameter. I would resolve this TODO and complete the javadoc here (and preferably for the method above too, to avoid confusion later)

Suggested change

* TODO Doc.

* @param getFlattenParents When true, the list of known parent and grand parent summaries will be resolved.

* This can have significant impact on performance, better be used only on manageable list (filtered by displayName, ..).

* Gets filtered page of room summaries so that [insert use of filtered here]

*

* @param queryParams - tba

* @param pagedListConfig - tba

* @param sortOrder - tba

* @param getFlattenParents When true, the list of known parent and grand parent summaries will be resolved.

If you use tba you should keep the TODO

ericdecanini · 2022-06-03T12:46:04Z

vector/src/main/java/im/vector/app/features/home/room/list/RoomSummaryItemFactory.kt

-        val spaceName = roomSummary.spaceParents?.firstOrNull()?.roomSummary?.name
+        val spaceName = roomSummary.flattenParents
+                .takeIf { it.isNotEmpty() }
+                ?.joinToString(", ") { it.name }


Although this is nice, I'm thinking now to maintain parity with web, we only take the last element in flattenParents for the subtitle. If design would deem it's better to have all parents separated by comma, we can have a meta ticket to change that across all platforms

Doesn't look to me what web is taking the last one only. What are the requirement exactly?
Can you take over it as I believe you are the one that initialy developed it?

fedrunov · 2022-06-06T07:31:15Z

@BillCarsonFr were you able to reproduce or you just assume that flattened parents is the cause? I've tested with many spaces and there were no problems with db read, so if you can reproduce, could you share your setup?

Also, issue was created before flatten parents flag was added (PR). So I'm not sure this actually fixes problem

BillCarsonFr · 2022-06-06T09:56:13Z

@BillCarsonFr were you able to reproduce or you just assume that flattened parents is the cause? I've tested with many spaces and there were no problems with db read, so if you can reproduce, could you share your setup?

Also, issue was created before flatten parents flag was added (PR). So I'm not sure this actually fixes problem

I can reproduce 100% with my personal account.

bmarty

Please add a file for the changelog

bmarty

LGTM after a static review.
Please do not merge on develop, I will create a hotfix, as it's blocking the release 1.4.18.

…space parent

ericdecanini · 2022-06-07T10:44:38Z

(force push to revert commit made on the wrong branch)

sonarqubecloud · 2022-06-07T11:20:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

bmarty · 2022-06-07T13:19:28Z

All the 4 commits have been cherry-picked to create the hotfix 1.4.19. The changes are now on develop and on main.

waclaw66 · 2022-06-09T18:01:44Z

Another regression #6272 that didn't make it to 1.4.19...

BillCarsonFr requested review from ericdecanini, bmarty and fedrunov June 3, 2022 12:27

BillCarsonFr added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Jun 3, 2022

ericdecanini suggested changes Jun 3, 2022

View reviewed changes

bmarty reviewed Jun 7, 2022

View reviewed changes

BillCarsonFr added 3 commits June 7, 2022 09:39

fix slow paged room list

b8eef9b

Fix param name

43b76f7

code review

adf8c5f

BillCarsonFr force-pushed the feature/bca/fix_slow_space_switch_regression branch from 3dfeeda to adf8c5f Compare June 7, 2022 07:39

bmarty approved these changes Jun 7, 2022

View reviewed changes

Changes space subtitle in explore rooms from joined string to direct …

89e528d

…space parent

ericdecanini approved these changes Jun 7, 2022

View reviewed changes

fedrunov approved these changes Jun 7, 2022

View reviewed changes

ericdecanini force-pushed the feature/bca/fix_slow_space_switch_regression branch from 3835755 to 89e528d Compare June 7, 2022 10:44

bmarty mentioned this pull request Jun 7, 2022

[Release] Element Android v1.4.18 #6206

Closed

61 tasks

bmarty closed this Jun 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix slow paged room list regression #6233

Fix slow paged room list regression #6233

BillCarsonFr commented Jun 3, 2022

github-actions bot commented Jun 3, 2022 •

edited

Loading

ericdecanini left a comment

ericdecanini Jun 3, 2022

bmarty Jun 7, 2022

ericdecanini Jun 3, 2022

bmarty Jun 7, 2022

ericdecanini Jun 3, 2022

BillCarsonFr Jun 3, 2022

fedrunov commented Jun 6, 2022 •

edited

Loading

BillCarsonFr commented Jun 6, 2022

bmarty left a comment

bmarty left a comment

ericdecanini commented Jun 7, 2022

sonarqubecloud bot commented Jun 7, 2022

bmarty commented Jun 7, 2022

waclaw66 commented Jun 9, 2022

	getFlattenParents: Boolean = false,
	getFlattenParents: Boolean = false, // Heavily impacts performance when true. Only use when space parent is absolutely required

Fix slow paged room list regression #6233

Fix slow paged room list regression #6233

Conversation

BillCarsonFr commented Jun 3, 2022

Type of change

Content

Tested devices

Checklist

github-actions bot commented Jun 3, 2022 • edited Loading

Unit Test Results

ericdecanini left a comment

Choose a reason for hiding this comment

ericdecanini Jun 3, 2022

Choose a reason for hiding this comment

bmarty Jun 7, 2022

Choose a reason for hiding this comment

ericdecanini Jun 3, 2022

Choose a reason for hiding this comment

bmarty Jun 7, 2022

Choose a reason for hiding this comment

ericdecanini Jun 3, 2022

Choose a reason for hiding this comment

BillCarsonFr Jun 3, 2022

Choose a reason for hiding this comment

fedrunov commented Jun 6, 2022 • edited Loading

BillCarsonFr commented Jun 6, 2022

bmarty left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ericdecanini commented Jun 7, 2022

sonarqubecloud bot commented Jun 7, 2022

bmarty commented Jun 7, 2022

waclaw66 commented Jun 9, 2022

github-actions bot commented Jun 3, 2022 •

edited

Loading

fedrunov commented Jun 6, 2022 •

edited

Loading