[C-2886] Improve cache performance by dylanjeffers · Pull Request #3792 · AudiusProject/audius-client

dylanjeffers · 2023-07-24T23:05:25Z

Description

Removes add action and uses redux-thunk to check confirmer before calling addSucceeded action
Improves/fixes fast cache option, using a smaller/configurable TTL. This will help prevent thrashing with updating a bunch of values over and over. ADD_SUCCESSFUL action time goes way down with this fix over an average session.
Adds "simple" cache option to prevent a ton of rerenders due to cache subscriptions. Doesn't remove this entirely so we can potentially re-implement this. It would be helpful to have subscriptions which would could work in conjunction with TTL.

amendelsohn

Wow, very excited to see this in action!!
Excellent job 🚀

amendelsohn · 2023-07-24T23:33:44Z

  [IntKeys.GATED_TRACK_POLL_INTERVAL_MS]: 1000,
-  [IntKeys.DISCOVERY_NOTIFICATIONS_GENESIS_UNIX_TIMESTAMP]: 0
+  [IntKeys.DISCOVERY_NOTIFICATIONS_GENESIS_UNIX_TIMESTAMP]: 0,
+  [IntKeys.CACHE_ENTRY_TTL]: 0


is 0 a safe default? Would that invalidate everything immediately?

what's potentially really silly, is that this makes it falsy, so the DEFAULT value in the code gets used. BUT now that you have me thinking, we should just get rid of the constant and use this.

amendelsohn · 2023-07-24T23:37:48Z

+      // If something is confirming and in the cache, we probably don't
+      // want to replace it (unless explicit) because we would lose client
+      // state, e.g. "has_current_user_reposted"


will this prevent us from doing multiple local updates to the local cache, or is it ok because those would always go through update not add?

yeah exactly ^ (also this is migrated line-for-line by the original saga that did this)

amendelsohn · 2023-07-24T23:41:39Z

        entity.metadata,
        mergeCustomizer
      )
-      newMetadata = updateImageCache(existing, entity.metadata, newMetadata)


safe to remove this?

oh great find, whew, no not at all!

amendelsohn · 2023-07-24T23:46:39Z

-    },
-    action: { ids: any[] }
-  ) {
+  [REMOVE_SUCCEEDED](state: CacheState, action: { ids: any[] }) {


love this CacheState change!

amendelsohn · 2023-07-24T23:48:05Z


-      dispatch(cacheActions.add(Kind.TRACKS, cacheTracks, false, true))
+      // // @ts-expect-error
+      // dispatch(cacheActions.add(Kind.TRACKS, cacheTracks, false, true))


can remove?

oh great find as well, no

amendelsohn · 2023-07-24T23:48:12Z


-      dispatch(cacheActions.add(Kind.TRACKS, cacheTracks, false, true))
+      // // @ts-expect-error
+      // dispatch(cacheActions.add(Kind.TRACKS, cacheTracks, false, true))


can remove?

thanks, was testing something out when i removed them, bringing back

amendelsohn · 2023-07-24T23:49:37Z

 } from '@audius/common'
 import { cacheActions, Kind, makeUid } from '@audius/common'
-import { call, select, put } from 'typed-redux-saga'
+import { call, put, select } from 'typed-redux-saga'


amendelsohn · 2023-07-24T23:50:04Z

  confirmTransaction,
-  RequestConfirmationError
+  RequestConfirmationError,
+  put


I worry this will get confusing and people will forget. Do we only need it when doing cache adds? What happens when we forget?

yeah it's super confusing. basically folks should literally not need to use it, it was just that redux-saga types don't know how to deal with dispatch allowing a ThunkAction. so i think will only be needed for the few times we call put(add(...))

hmm ok I guess that's fine. Wondering if there's a place we can put a comment so someone dealing with the type error won't waste time on it

added a comment :)

amendelsohn · 2023-07-24T23:51:22Z

-export function* add(kind, entries, replace, persist) {
-  // Get cached things that are confirming
-  const confirmCalls = yield select(getConfirmCalls)
-  const cache = yield select(getCache, { kind })
-  const confirmCallsInCache = pick(
-    cache.entries,
-    Object.keys(confirmCalls).map((kindId) => getIdFromKindId(kindId))
-  )
-
-  const entriesToAdd = []
-  const entriesToSubscribe = []
-  entries.forEach((entry) => {
-    // If something is confirming and in the cache, we probably don't
-    // want to replace it (unless explicit) because we would lose client
-    // state, e.g. "has_current_user_reposted"
-    if (!replace && entry.id in confirmCallsInCache) {
-      entriesToSubscribe.push({ uid: entry.uid, id: entry.id })
-    } else {
-      entriesToAdd.push(entry)
-    }
-  })
-  if (entriesToAdd.length > 0) {
-    yield put(
-      cacheActions.addSucceeded({
-        kind,
-        entries: entriesToAdd,
-        replace,
-        persist
-      })
-    )
-  }
-  if (entriesToSubscribe.length > 0) {
-    yield put(cacheActions.subscribe(kind, entriesToSubscribe))
-  }
-}
-
-// Adds entries but first checks if they are confirming.
-// If they are, don't add or else we could be in an inconsistent state.
-function* watchAdd() {
-  yield takeEvery(cacheActions.ADD, function* (action) {
-    const { kind, entries, replace, persist } = action
-    yield call(add, kind, entries, replace, persist)
-  })
-}
-


amendelsohn · 2023-07-24T23:53:08Z

-    yield put(cacheActions.setCacheType({ cacheType: 'fast' }))
+  let cacheType = 'normal'
+
+  if (fastCache || (isNativeMobile && fastMobileCache)) {


do we want to be able to enable it for web but not mobile? seems like we can either do both or just mobile this way

thanks for the callout here, i realized we dont need a separate "fast" and fast-mobile" now that desktop issues are fixed. this was to allow us to roll forward with the fast cache on mobile cause desktop was acting up for a while there. but we are consistent now, so will remove.

audius-infra · 2023-07-25T18:25:47Z

Preview this change https://demo.audius.co/dj-cache-improvements

This reverts commit 5af77ec.

[5e99303] Add favorite test and fix aria-label (#3817) Raymond Jacobson [ccc32ce] [C-2908 C-2744] fix desktop follow button (#3816) Dylan Jeffers [c0679c3] [PAY-1660] Fix layout issues with TrackTile socials row with a lot of stats (#3815) Randy Schott [089a9e6] Pin stripe package versions (#3813) Reed [a281267] [C-2774] Update upload inputs (#3806) Dylan Jeffers [f504ef9] [C-2901] Fix menu types (#3811) Dylan Jeffers [cb9a385] [C-2905] Update Text types and props to camelCase (#3810) Kyle Shanks [027b3a5] [PAY-1624] Implement Purchase modal (#3808) Randy Schott [deadb5f] [C-2902] Update the upload forms to use the typography component (#3809) Kyle Shanks [039c951] [C-801] Fix oauth nodes (#3807) Raymond Jacobson [c3765c7] Update typography component to use classnames (#3805) Kyle Shanks [cab0a3e] Switch to Stripe package instead of script (#3798) Reed [a84126f] [C-2890] Add first version of a typography component to web (#3796) Kyle Shanks [4addddc] Fix mobile prem-content drawer unlocking margin (#3804) Reed [233b585] [C-2857] Remove get blocknumber (#3802) Dylan Jeffers [d113bdb] Prepare for 1.5.34 full app release (#3801) Dylan Jeffers [2f09db4] [C-2887] Fix collection button widths (#3800) Dylan Jeffers [8158e10] [PAY-1655] Add ColorValue prop to Text component (#3799) Reed [fe4bc6a] Revert cacheActions.add thunk (#3797) Dylan Jeffers [2370bbe] [PAY-1650] Update play/preview buttons on track details to use HarmonyButton (#3795) Randy Schott [3579dc2] [PAY-1651] Implements Harmony Buttons (#3794) Randy Schott [5af77ec] [C-2886] Improve cache performance (#3792) Dylan Jeffers [6fb78f1] [PAY-1587] Mobile USDC Purchase Drawer Skeleton (#3793) Reed [1277a41] [C-2883] Migrate confirmer to common (#3788) Dylan Jeffers [ce2548e] [plat-1111] add usdc purchase seller and buyer notifications (#3770) sabrina-kiam [bc04f52] Fix mobile LockedStatusBadge padding (#3790) Reed [8943078] [C-2680] Attribution Modal (#3778) Andrew Mendelsohn

dylanjeffers added 8 commits July 20, 2023 15:09

Cache improvements

280ba2b

Migrate confirmer

400770a

Merge branch 'main' into dj-migrate-confirmer

8e64f68

Remove log

c4fd64d

Finish

de600a5

Merge branch 'dj-migrate-confirmer' into dj-cache-improvements

9093ed4

Improve cache/add

298116f

Merge branch 'main' into dj-cache-improvements

d7c1469

pull-request-size Bot added the size/L label Jul 24, 2023

dylanjeffers requested review from a team and amendelsohn and removed request for a team July 24, 2023 23:05

amendelsohn approved these changes Jul 24, 2023

View reviewed changes

Address comments

de7bb83

dylanjeffers added 3 commits July 25, 2023 11:41

Final things

2d5662d

What is happening

f17ef24

Revert name

75192e6

dylanjeffers merged commit 5af77ec into main Jul 25, 2023

dylanjeffers deleted the dj-cache-improvements branch July 25, 2023 18:46

dylanjeffers added a commit that referenced this pull request Jul 25, 2023

Revert "[C-2886] Improve cache performance (#3792)"

53e9a5d

This reverts commit 5af77ec.

AudiusProject deleted a comment from linear Bot Sep 11, 2023

Conversation

dylanjeffers commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

amendelsohn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

audius-infra commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dylanjeffers commented Jul 24, 2023 •

edited

Loading