[Comments] Batch resolve #7236

2021-10-06T14:59:42+02:00

saltrafael commented

2021-10-06 14:59:42 +02:00

(Migrated from github.com)

Fixes

Now comments, and replies when expanded, get batch-resolved, and avoid getting re-resolved on components like channelThumbnail or uriIndicator. There is no issue for it but all those resolve requests are unnecessary, also does this help with CPU stuff (#6473, #7177) @infinite-persistence ?

## Fixes Now comments, and replies when expanded, get batch-resolved, and avoid getting re-resolved on components like channelThumbnail or uriIndicator. There is no issue for it but all those resolve requests are unnecessary, also does this help with CPU stuff (#6473, #7177) @infinite-persistence ?

infinite-persistence (Migrated from github.com) reviewed 2021-10-08 16:56:13 +02:00

infinite-persistence (Migrated from github.com) left a comment

Initial comments below. Will look deeper tomorrow.

ui/component/commentsList/index.js

infinite-persistence (Migrated from github.com) commented

2021-10-08 16:43:50 +02:00

This feels heavy because:

makings copies of arrays
double loop (map and filter)
loop increases with comment count
and connect gets called every time there is a dispatched action or store change. e.g. Refreshing the comments to page 1 alone called this loop 5-6 times, which is why these heavy transformations are done in a memoized selector.

I think the idea of batch-resolving at CommentList is good, but wondering if there is a leaner way to do it.

Suggestion:

Instead of the transformation above and passing alreadyResolved to several components, can we try to use the cache-aware doResolveUris(uris, true) for those affected components? Then, no extra code will be needed as the checking will be encapsulated in doResolveUris. This also avoids adding more props, especially for Comments which is getting out of hand.

This feels heavy because: - makings copies of arrays - double loop (map and filter) - loop increases with comment count - and `connect` gets called every time there is a dispatched action or store change. e.g. Refreshing the comments to page 1 alone called this loop 5-6 times, which is why these heavy transformations are done in a memoized selector. I think the idea of batch-resolving at `CommentList` is good, but wondering if there is a leaner way to do it. Suggestion: - Instead of the transformation above and passing `alreadyResolved` to several components, can we try to use the cache-aware `doResolveUris(uris, true)` for those affected components? Then, no extra code will be needed as the checking will be encapsulated in `doResolveUris`. This also avoids adding more props, especially for `Comments` which is getting out of hand.

infinite-persistence (Migrated from github.com) reviewed 2021-10-09 14:30:27 +02:00

ui/component/commentsList/index.js

infinite-persistence (Migrated from github.com) commented

2021-10-09 14:30:27 +02:00

Probably don't need to handle pinned comments too (let it resolve individually) -- the count is usually low, so we can save one transformation and just batch-resolve topLevelComments

Probably don't need to handle pinned comments too (let it resolve individually) -- the count is usually low, so we can save one transformation and just batch-resolve `topLevelComments`

saltrafael (Migrated from github.com) reviewed 2021-10-10 14:29:32 +02:00

ui/component/commentsList/index.js

saltrafael (Migrated from github.com) commented

2021-10-10 14:29:32 +02:00

done! Looks much cleaner now

infinite-persistence (Migrated from github.com) reviewed 2021-10-11 03:29:11 +02:00

ui/component/commentsList/index.js

					
				@ -27,2 +37,4 @@

				  return {

				    topLevelComments,

				    resolvedComments,

				    myChannels: selectMyChannelClaims(state),

infinite-persistence (Migrated from github.com) commented

2021-10-11 03:29:11 +02:00

resolveComments = topLevelComments.filter() might be a clearer choice to convey the intention, but that's minor.

`resolveComments = topLevelComments.filter()` might be a clearer choice to convey the intention, but that's minor.

infinite-persistence (Migrated from github.com) approved these changes 2021-10-11 03:29:19 +02:00

infinite-persistence commented

2021-10-11 03:31:22 +02:00

(Migrated from github.com)

a0a9828 to 620de2e

Rebase to ody master

a0a9828 to 620de2e - Rebase to ody master

Sign in to join this conversation.

No reviewers

infinite-persistence

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-desktop#7236