[#18608] Collectibles fetching performance #18921
Conversation
| (def ->kebab-case-keyword (memoize csk/->kebab-case-keyword)) | ||
| (def ->PascalCaseKeyword (memoize csk/->PascalCaseKeyword)) | ||
|
|
There was a problem hiding this comment.
Memoized version to convert keys 👀
| :wallet/all-collectibles | ||
| :wallet/all-collectibles-list | ||
| :<- [:wallet] | ||
| (fn [wallet] | ||
| (->> wallet | ||
| :accounts | ||
| (mapcat (comp :collectibles val)) | ||
| (add-collectibles-preview-url)))) | ||
| (fn [{:keys [accounts]}] | ||
| (let [max-collectibles (->> accounts | ||
| (map (comp count :collectibles val)) | ||
| (apply max)) | ||
| all-collectibles (map (fn [[_address {:keys [collectibles]}]] | ||
| (let [amount-to-add (- max-collectibles (count collectibles)) | ||
| empty-collectibles (repeat amount-to-add nil)] | ||
| (reduce conj collectibles empty-collectibles))) | ||
| accounts)] | ||
| (->> all-collectibles | ||
| (apply interleave) | ||
| (remove nil?) | ||
| (add-collectibles-preview-url))))) |
There was a problem hiding this comment.
The sub updated to return the listing in a different order
There was a problem hiding this comment.
I believe the idea is to display one collectible from each account in order (with a workaround to place nil for accounts with fewer collectibles and filtering them)
There was a problem hiding this comment.
Hey @smohamedjavid
Yeah, that's correct, I tried to explain this with an example in the description 😅
| @@ -25,24 +25,8 @@ | |||
| (is (match? result-db expected-db)))))) | |||
|
|
|||
| (deftest store-collectibles | |||
| :window-size 11 | ||
| :num-columns 2 | ||
| :render-fn (fn [{:keys [preview-url] :as collectible}] | ||
| [quo/collectible | ||
| {:images [preview-url] | ||
| :on-press #(when on-collectible-press | ||
| (on-collectible-press collectible))}]) | ||
| :on-end-reached on-end-reached | ||
| :on-end-reached-threshold 4}]))) |
There was a problem hiding this comment.
Changed the window size (from 21 to 11) to render less collectibles, hopefully this will increase the performance since less images have to be rendered.
Also added an on-end-reached fn to trigger the request for more collectibles,
Jenkins BuildsClick to see older builds (16)
|
There was a problem hiding this comment.
Took a while to understand the logic. 😄
Nice work as always! @ulisesmac 🚀
| :wallet/all-collectibles | ||
| :wallet/all-collectibles-list | ||
| :<- [:wallet] | ||
| (fn [wallet] | ||
| (->> wallet | ||
| :accounts | ||
| (mapcat (comp :collectibles val)) | ||
| (add-collectibles-preview-url)))) | ||
| (fn [{:keys [accounts]}] | ||
| (let [max-collectibles (->> accounts | ||
| (map (comp count :collectibles val)) | ||
| (apply max)) | ||
| all-collectibles (map (fn [[_address {:keys [collectibles]}]] | ||
| (let [amount-to-add (- max-collectibles (count collectibles)) | ||
| empty-collectibles (repeat amount-to-add nil)] | ||
| (reduce conj collectibles empty-collectibles))) | ||
| accounts)] | ||
| (->> all-collectibles | ||
| (apply interleave) | ||
| (remove nil?) | ||
| (add-collectibles-preview-url))))) |
There was a problem hiding this comment.
I believe the idea is to display one collectible from each account in order (with a workaround to place nil for accounts with fewer collectibles and filtering them)
| :justify-content :center | ||
| :align-items :center | ||
| :background-color :lightblue)} | ||
| [text/text "SVG Content"]] |
There was a problem hiding this comment.
Can we create an issue (if there is not one created yet) for researching SVG support and reimplement it and link it in a comment?
There was a problem hiding this comment.
We already have an issue for it, it's about support all types of collectibles, I've found other library (react native skia) and we should experiment:
#18657
There was a problem hiding this comment.
Great work @ulisesmac! No issues from my side, hope QA can confirm the boost in collectible fetching performance 🚀
94% of end-end tests have passedFailed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (45)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
|
ulisesmac
force-pushed
the
18608-collectibles-performance
branch
from
6dfb87d
to
4f29c08
Compare
|
Hi @ulisesmac, could you please check if this issue is related to the current PR? It's interesting because it's reproducible only for this specific user and with the current PR. The user has a lot of data (communities, contacts, wallets), but it's not reproducible for other users who also have a lot of data. Initially, I thought it might be related to an old issue, but in the old issue #18738, it was reproducible randomly and very rarely, unlike in the current PR it happens every time I try to log in. ISSUE 1: Frozen login screen occurs after specific user is logged inSteps:
Actual result:The login screen becomes frozen, and the buttons are unresponsive. login.mp4Expected result:The buttons on the login screen should remain responsive after logout, allowing the user to interact with them. Logs: |
|
Today, I'm not super helpful in debugging :( ISSUE 2: [Android] "No protocol method IMapEntry.-key..." Error is shown after collectibles are quickly scrolledSteps:
Actual result:"No protocol method IMapEntry.-key..." Error is shown after collectibles are quickly scrolled collectibles.mp4Expected result:No errors when collectibles are scrolled Device:Pixel 7a, Android 13 Logs: |
ulisesmac
force-pushed
the
18608-collectibles-performance
branch
from
636bc5e
to
f8d6035
Compare
|
Hey @VolodLytvynenko Thanks for testing!
I've tested the app on the latest commit on develop, and this issue with this account is reproducible, it means this PR is not involved. The "freeze" happens even before the collectibles are requested. Actually, this error seems to be related to some callbacks from status go that are taking some time. Probably due to a lot of info?
Thanks for noticing it. It happened when a collectible request failed, it failed when a request with an ID was in progress and we dispatched another request with the same ID. Now this implementation generates different IDs for all requests and in case of failure, handles the errors properly. |
90% of end-end tests have passedFailed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (43)Click to expandClass TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
Hi @ulisesmac thank you for PR. No issues from my side. PR is ready to be merged |
ulisesmac
force-pushed
the
18608-collectibles-performance
branch
from
208c443
to
2941746
Compare
fixes #18608
fixes #18614
Summary
This PR improves the performance of the collectible fetching and storing by changing the strategy to request them, it gives similar results in terms of UX.
Recorded in a development build, release should be even faster.
Before (UI freezed):
Screencast.from.2024-02-20.15-55-34.webm
Now:
Screencast.from.2024-02-20.14-40-16.webm
Details
(if you are interested)
This PR:
Adds a memoized version of key transformation functions.
I've tried several approaches, also implementing a custom key-value cache based on a JS object (similar to reagent's one). but I realized just memoizing is the best.
I did some local measurements (not too strict), I was getting 38 - 45ms and now I'm getting 7 - 21 ms, so this is definitely an improvement.
Remove the buggy SVG collectibles.
They've been too problematic, they fill the console with warnings, and don't properly understand the svgs received. I've seen React Native Skia, we can give it a try. Now I'm just showing a placeholder for these collectibles, as well as for the missing ones. We must implement a proper collectible component.
Remove the
displayable-collectibles?function.According to designs, all collectibles should be displayed, but some of them aren't unsupported.
(The actual issue) Instead of requesting all the owned collectibles by batches:
Reduce the batch size (from 1000 to 25)
Only request one batch when the user logs in, the remaining will be requested as the user scrolls the listings.
Divides the batch size (25) by the number of accounts and perform individual request to get the collectibles for each account evenly. (e.g. 25 collectibles and 5 accounts mean 5 collectibles per account). This is only applied in home screen, when we visit an account, all those 25 collectibles are for the visited account.
Stores the collectibles fetched per account in a separate re-frame key (
:ui :collectibles :fetched), once all requests have finished, writes the collectibles into their accounts. The reason is the view is subscribed, if we update each time we receive. we'll get many re-renders, besides performance, it doesn't look good in terms of UX.Change the subscription to list all the collectibles in the home page:
This is a listing:
So we should add new received collectibles at the end of the listing to have a nice UX, also flat list behaves weird (jumps) if new items are added in the middle. Currently we are adding in the middle, this PR changes the sub to add at the end.
E.g. Currently, if we have
account-1andaccount-2collectibles, the list would look as:acc-1--coll-1acc-1--coll-2acc-1--coll-3acc-2--coll-1acc-2--coll-2acc-2--coll-3If we receive more collectibles, for each account then they will be inserted in the middle of the listing:
acc-1--coll-1acc-1--coll-2acc-1--coll-3acc-1--coll-4<- Here!acc-2--coll-1acc-2--coll-2acc-2--coll-3acc-2--coll-4<- Here!So this PR changes the sub to return:
acc-1--coll-1acc-2--coll-1acc-1--coll-2acc-2--coll-2acc-1--coll-3acc-2--coll-3New collectibles will be added to the end, and don't worry, it properly handles accounts containing a different amount of collectibles.
Testing notes
Make sure to add the mentioned addreses (in this PR's issue) to test the performance. Sometimes even if you added them, status-go doesn't return the collectibles, maybe you'll need to wait (idk for how long) after adding them.
Platforms
Non-functional
Steps to test
status: ready