From c97cab0ebb292b90f2188978d413ad0a14c8fccc Mon Sep 17 00:00:00 2001 From: infinite-persistence <64950861+infinite-persistence@users.noreply.github.com> Date: Tue, 9 Nov 2021 01:25:29 +0800 Subject: [PATCH] `byId[]` fixes to reduce invalidation (#239) * Don't update 'pendingById' if no changes. 'pendingById' isn't frequently updated, but using it as a proof-of-concept to fix how reducers should be written to avoid unnecessary updates. ImmutableJS apparently does all of this for us, but there are cons to using it as well, so using own wrappers for now. * Don't update 'byId' if no changes + add 'selectClaimWithId' ## Ticket 116 Claim store optimization ideas (reducing unnecessary renders) ## Changes - Ignore things like `confirmations` so that already-fetched claims aren't invalidated and causes re-rendering. The `stringify` might look expensive, but the amount of avoided re-renders outweighs it. There might be faster ways to compare, though. - With `byId[claimId]` references more stable now, memoized selectors can now use 'selectClaimWithId' to pick a specific claim to depend on, instead of 'byId' which changes on every update. * Fix memo: selectMyChannelClaims, selectActiveChannelClaim ## Issue These should never recalculate after `channel_list` has been fetched, but they do because of poor selector dependency. ## Change With the `byId` changes from the previous commit, we are now able to memoize these selectors correctly. --- ui/redux/reducers/claims.js | 104 +++++++++++++++++++++++++++++------ ui/redux/selectors/app.js | 10 ++-- ui/redux/selectors/claims.js | 37 +++++++++++-- 3 files changed, 123 insertions(+), 28 deletions(-) diff --git a/ui/redux/reducers/claims.js b/ui/redux/reducers/claims.js index 1c321d351..af6a5032d 100644 --- a/ui/redux/reducers/claims.js +++ b/ui/redux/reducers/claims.js @@ -111,11 +111,63 @@ const defaultState = { checkingReflecting: false, }; +// **************************************************************************** +// Helpers +// **************************************************************************** + +function isObjEmpty(object: any) { + return Object.keys(object).length === 0; +} + +function resolveDelta(original: any, delta: any) { + if (isObjEmpty(delta)) { + // Don't invalidate references when there are no changes, so return original. + return original; + } else { + // When there are changes: create a new object, spread existing references, + // and overwrite specific items with new data. + return { ...original, ...delta }; + } +} + +function claimObjHasNewData(original, fresh) { + // Don't blow away 'is_my_output' just because the next query didn't ask for it. + const ignoreIsMyOutput = original.is_my_output !== undefined && fresh.is_my_output === undefined; + + // Something is causing the tags to be re-ordered differently + // (https://github.com/OdyseeTeam/odysee-frontend/issues/116#issuecomment-962747147). + // Just do a length comparison for now, which covers 99% of cases while we + // figure out what's causing the order to change. + const ignoreTags = + original.value && + fresh.value && + original.value.tags && + fresh.value.tags && + original.value.tags.length !== fresh.value.tags.length; + + const excludeKeys = (key, value) => { + if (key === 'confirmations' || (ignoreTags && key === 'tags') || (ignoreIsMyOutput && key === 'is_my_output')) { + return undefined; + } + + return value; + }; + + const originalStringified = JSON.stringify(original, excludeKeys); + const freshStringified = JSON.stringify(fresh, excludeKeys); + + return originalStringified !== freshStringified; +} + +// **************************************************************************** +// handleClaimAction +// **************************************************************************** + function handleClaimAction(state: State, action: any): State { const { resolveInfo }: ClaimActionResolveInfo = action.data; const byUri = Object.assign({}, state.claimsByUri); - const byId = Object.assign({}, state.byId); + const byIdDelta = {}; const channelClaimCounts = Object.assign({}, state.channelClaimCounts); const pendingById = state.pendingById; let newResolvingUrls = new Set(state.resolvingUris); @@ -128,10 +180,13 @@ function handleClaimAction(state: State, action: any): State { if (stream) { if (pendingById[stream.claim_id]) { - byId[stream.claim_id] = mergeClaim(stream, byId[stream.claim_id]); + byIdDelta[stream.claim_id] = mergeClaim(stream, state.byId[stream.claim_id]); } else { - byId[stream.claim_id] = stream; + if (!state.byId[stream.claim_id] || claimObjHasNewData(state.byId[stream.claim_id], stream)) { + byIdDelta[stream.claim_id] = stream; + } } + byUri[url] = stream.claim_id; // If url isn't a canonical_url, make sure that is added too @@ -158,9 +213,9 @@ function handleClaimAction(state: State, action: any): State { } if (pendingById[channel.claim_id]) { - byId[channel.claim_id] = mergeClaim(channel, byId[channel.claim_id]); + byIdDelta[channel.claim_id] = mergeClaim(channel, state.byId[channel.claim_id]); } else { - byId[channel.claim_id] = channel; + byIdDelta[channel.claim_id] = channel; } byUri[channel.permanent_url] = channel.claim_id; @@ -171,10 +226,11 @@ function handleClaimAction(state: State, action: any): State { if (collection) { if (pendingById[collection.claim_id]) { - byId[collection.claim_id] = mergeClaim(collection, byId[collection.claim_id]); + byIdDelta[collection.claim_id] = mergeClaim(collection, state.byId[collection.claim_id]); } else { - byId[collection.claim_id] = collection; + byIdDelta[collection.claim_id] = collection; } + byUri[url] = collection.claim_id; byUri[collection.canonical_url] = collection.claim_id; byUri[collection.permanent_url] = collection.claim_id; @@ -193,7 +249,7 @@ function handleClaimAction(state: State, action: any): State { }); return Object.assign({}, state, { - byId, + byId: resolveDelta(state.byId, byIdDelta), claimsByUri: byUri, channelClaimCounts, resolvingUris: Array.from(newResolvingUrls), @@ -201,6 +257,10 @@ function handleClaimAction(state: State, action: any): State { }); } +// **************************************************************************** +// Reducers +// **************************************************************************** + reducers[ACTIONS.RESOLVE_URIS_STARTED] = (state: State, action: any): State => { const { uris }: { uris: Array } = action.data; @@ -237,7 +297,8 @@ reducers[ACTIONS.FETCH_CLAIM_LIST_MINE_COMPLETED] = (state: State, action: any): const byId = Object.assign({}, state.byId); const byUri = Object.assign({}, state.claimsByUri); - const pendingById = Object.assign({}, state.pendingById); + const pendingByIdDelta = {}; + let myClaimIds = new Set(state.myClaims); let urlsForCurrentPage = []; @@ -245,8 +306,10 @@ reducers[ACTIONS.FETCH_CLAIM_LIST_MINE_COMPLETED] = (state: State, action: any): const { permanent_url: permanentUri, claim_id: claimId, canonical_url: canonicalUri } = claim; if (claim.type && claim.type.match(/claim|update/)) { urlsForCurrentPage.push(permanentUri); + if (claim.confirmations < 1) { - pendingById[claimId] = claim; + pendingByIdDelta[claimId] = claim; + if (byId[claimId]) { byId[claimId] = mergeClaim(claim, byId[claimId]); } else { @@ -255,6 +318,7 @@ reducers[ACTIONS.FETCH_CLAIM_LIST_MINE_COMPLETED] = (state: State, action: any): } else { byId[claimId] = claim; } + byUri[permanentUri] = claimId; byUri[canonicalUri] = claimId; myClaimIds.add(claimId); @@ -265,7 +329,7 @@ reducers[ACTIONS.FETCH_CLAIM_LIST_MINE_COMPLETED] = (state: State, action: any): isFetchingClaimListMine: false, myClaims: Array.from(myClaimIds), byId, - pendingById, + pendingById: resolveDelta(state.pendingById, pendingByIdDelta), claimsByUri: byUri, myClaimsPageResults: urlsForCurrentPage, myClaimsPageNumber: page, @@ -279,7 +343,7 @@ reducers[ACTIONS.FETCH_CHANNEL_LIST_STARTED] = (state: State): State => reducers[ACTIONS.FETCH_CHANNEL_LIST_COMPLETED] = (state: State, action: any): State => { const { claims }: { claims: Array } = action.data; let myClaimIds = new Set(state.myClaims); - const pendingById = Object.assign({}, state.pendingById); + const pendingByIdDelta = {}; let myChannelClaims; const byId = Object.assign({}, state.byId); const byUri = Object.assign({}, state.claimsByUri); @@ -302,8 +366,10 @@ reducers[ACTIONS.FETCH_CHANNEL_LIST_COMPLETED] = (state: State, action: any): St // $FlowFixMe myChannelClaims.add(claimId); + if (confirmations < 1) { - pendingById[claimId] = claim; + pendingByIdDelta[claimId] = claim; + if (byId[claimId]) { byId[claimId] = mergeClaim(claim, byId[claimId]); } else { @@ -312,13 +378,14 @@ reducers[ACTIONS.FETCH_CHANNEL_LIST_COMPLETED] = (state: State, action: any): St } else { byId[claimId] = claim; } + myClaimIds.add(claimId); }); } return Object.assign({}, state, { byId, - pendingById, + pendingById: resolveDelta(state.pendingById, pendingByIdDelta), claimsByUri: byUri, channelClaimCounts, fetchingMyChannels: false, @@ -342,7 +409,7 @@ reducers[ACTIONS.FETCH_COLLECTION_LIST_COMPLETED] = (state: State, action: any): const { claims }: { claims: Array } = action.data; const myClaims = state.myClaims || []; let myClaimIds = new Set(myClaims); - const pendingById = Object.assign({}, state.pendingById); + const pendingByIdDelta = {}; let myCollectionClaimsSet = new Set([]); const byId = Object.assign({}, state.byId); const byUri = Object.assign({}, state.claimsByUri); @@ -357,9 +424,11 @@ reducers[ACTIONS.FETCH_COLLECTION_LIST_COMPLETED] = (state: State, action: any): // $FlowFixMe myCollectionClaimsSet.add(claimId); + // we don't want to overwrite a pending result with a resolve if (confirmations < 1) { - pendingById[claimId] = claim; + pendingByIdDelta[claimId] = claim; + if (byId[claimId]) { byId[claimId] = mergeClaim(claim, byId[claimId]); } else { @@ -368,6 +437,7 @@ reducers[ACTIONS.FETCH_COLLECTION_LIST_COMPLETED] = (state: State, action: any): } else { byId[claimId] = claim; } + myClaimIds.add(claimId); }); } @@ -375,7 +445,7 @@ reducers[ACTIONS.FETCH_COLLECTION_LIST_COMPLETED] = (state: State, action: any): return { ...state, byId, - pendingById, + pendingById: resolveDelta(state.pendingById, pendingByIdDelta), claimsByUri: byUri, fetchingMyCollections: false, myCollectionClaims: Array.from(myCollectionClaimsSet), diff --git a/ui/redux/selectors/app.js b/ui/redux/selectors/app.js index 923556e72..37f6786ba 100644 --- a/ui/redux/selectors/app.js +++ b/ui/redux/selectors/app.js @@ -1,5 +1,5 @@ import { createSelector } from 'reselect'; -import { selectClaimsById, selectMyChannelClaims, makeSelectStakedLevelForChannelUri } from 'redux/selectors/claims'; +import { selectClaimWithId, selectMyChannelClaims, makeSelectStakedLevelForChannelUri } from 'redux/selectors/claims'; export const selectState = (state) => state.app || {}; @@ -70,15 +70,13 @@ export const selectSplashAnimationEnabled = (state) => selectState(state).splash export const selectActiveChannelId = (state) => selectState(state).activeChannel; export const selectActiveChannelClaim = createSelector( - selectActiveChannelId, - selectClaimsById, + (state) => selectClaimWithId(state, selectActiveChannelId(state)), // i.e. 'byId[activeChannelId]' specifically, instead of just 'byId'. selectMyChannelClaims, - (activeChannelClaimId, claimsById, myChannelClaims) => { - if (!activeChannelClaimId || !claimsById || !myChannelClaims || !myChannelClaims.length) { + (activeChannelClaim, myChannelClaims) => { + if (!activeChannelClaim || !myChannelClaims || !myChannelClaims.length) { return undefined; } - const activeChannelClaim = claimsById[activeChannelClaimId]; if (activeChannelClaim) { return activeChannelClaim; } diff --git a/ui/redux/selectors/claims.js b/ui/redux/selectors/claims.js index 326e221a8..1ee07dd04 100644 --- a/ui/redux/selectors/claims.js +++ b/ui/redux/selectors/claims.js @@ -43,6 +43,21 @@ export const selectClaimsByUri = createSelector(selectClaimIdsByUri, selectClaim return claims; }); +/** + * Returns the claim with the specified ID. The claim could be undefined if does + * not exist or have not fetched. Take note of the second parameter, which means + * an inline function or helper would be required when used as an input to + * 'createSelector'. + * + * @param state + * @param claimId + * @returns {*} + */ +export const selectClaimWithId = (state: State, claimId: string) => { + const byId = selectClaimsById(state); + return byId[claimId]; +}; + export const selectAllClaimsByChannel = createSelector(selectState, (state) => state.paginatedClaimsByChannel || {}); export const selectPendingIds = createSelector(selectState, (state) => Object.keys(state.pendingById) || []); @@ -404,14 +419,26 @@ export const selectFetchingMyCollections = (state: State) => selectState(state). export const selectMyChannelClaimIds = (state: State) => selectState(state).myChannelClaims; -export const selectMyChannelClaims = createSelector(selectState, selectClaimsById, (state, byId) => { - const ids = state.myChannelClaims; - if (!ids) { - return ids; +export const selectMyChannelClaims = createSelector(selectMyChannelClaimIds, (myChannelClaimIds) => { + if (!myChannelClaimIds) { + return myChannelClaimIds; } + if (!window || !window.store) { + return undefined; + } + + // Note: Grabbing the store and running the selector this way is anti-pattern, + // but it is _needed_ and works only because we know for sure that 'byId[]' + // will be populated with the same claims as when 'myChannelClaimIds' is populated. + // If we put 'state' or 'byId' as the input selector, it essentially + // recalculates every time. Putting 'state' as input to createSelector() is + // always wrong from a memoization standpoint. + const state = window.store.getState(); + const byId = selectClaimsById(state); + const claims = []; - ids.forEach((id) => { + myChannelClaimIds.forEach((id) => { if (byId[id]) { // I'm not sure why this check is necessary, but it ought to be a quick fix for https://github.com/lbryio/lbry-desktop/issues/544 claims.push(byId[id]);