ref(replay): refactor replayerStepper calls to be memoized #74606
Conversation
github-actions
bot
added
the
Scope: Frontend
| import DomNodesChart from 'sentry/views/replays/detail/memoryPanel/domNodesChart'; | ||
| import MemoryChart from 'sentry/views/replays/detail/memoryPanel/memoryChart'; | ||
|
|
||
| function useCountDomNodes({replay}: {replay: null | ReplayReader}) { |
There was a problem hiding this comment.
moved to its own file, useCountDomNodes.tsx
There was a problem hiding this comment.
renamed to useExtractPageHtml.tsx
There was a problem hiding this comment.
renamed to useExtractDomNodes.tsx
| export default function extractDomNodes({ | ||
| frames, | ||
| rrwebEvents, | ||
| startTimestampMs, | ||
| }: Args): Promise<Map<ReplayFrame, Extraction>> { | ||
| return replayerStepper({ | ||
| frames, | ||
| rrwebEvents, | ||
| startTimestampMs, | ||
| shouldVisitFrame: frame => { | ||
| const nodeId = getNodeId(frame); | ||
| return nodeId !== undefined && nodeId !== -1; | ||
| }, | ||
| onVisitFrame: (frame, collection, replayer) => { | ||
| const mirror = replayer.getMirror(); | ||
| const nodeId = getNodeId(frame); | ||
| const html = extractHtml(nodeId as number, mirror); | ||
| collection.set(frame as ReplayFrame, { | ||
| frame, | ||
| html, | ||
| timestamp: frame.timestampMs, | ||
| }); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
moved to replayReader.tsx, so i renamed this file
There was a problem hiding this comment.
moved to replayReader.tsx, exported type was moved into the corresponding hook
|
It might be nice to have a loading indicator for the dom node chart |
@billyvg we had a placeholder before but seems our Screen.Recording.2024-07-23.at.12.12.23.PM.mov |
michellewzhang
force-pushed
the
mz/replayer-ref-v2
branch
from
b382322 to
fb68487
Compare
| window.setTimeout(() => { | ||
| const timestamp = | ||
| 'offsetMs' in frame ? frame.offsetMs : frame.timestamp - startTimestampMs; | ||
| replayer.pause(timestamp); | ||
| }, 0); |
There was a problem hiding this comment.
we could try it, and see if the memory chart renders the same as before.
also, we could experiment with different granularity in the memory chart.
There was a problem hiding this comment.
Here's an answer i found on the differences: https://stackoverflow.com/questions/38709923/why-is-requestanimationframe-better-than-setinterval-or-settimeout
There was a problem hiding this comment.
seems to be the same behavior after using RAF so i pushed up the change
Placeholder looks great! |
replayerStepperto be called less #74540, the experimental refactor ofreplayerStepperreplayerSteppercalls so that we remember the results and don't have to reload the data every time.current state of things:
we have distinct
replayerSteppercalls in the breadcrumbs + memory tabs. in total, we could be calling it 3 times because it's used by(1) the breadcrumbs tab to render the HTML snippet,
(2) for hydration error text diffs,
(3) and the DOM nodes chart.
the plan:
improve things by calling
replayerStepperonce (or at least less) and memoize the results. side note: it was hard to combine the hydration error text diffreplayerSteppercall with the others due to the types, so we had to keep that call separate.the question:
is it faster to run one
replayerStepperinstance, and collect all the data at one time (iterate over the frames exactly once)? this means the speed of any tab will always be limited by the slower call, since we're going through ALL the data at once, even some that we might not need yet. like this:sentry/static/app/utils/replays/replayReader.tsx
Lines 363 to 366 in 70c9054
or is it better to continue to have 2 stepper instances that iterate over 2 different sets of frames (breadcrumbs loops over hundreds of frames, and DOM node count iterates over thousands, which means the speeds could be drastically different). in this situation, each tab would handle their own array of necessary frames.
the experiments:
the first PR (#74540) tried to explore a refactor where
replayerStepperis called once for the two DOM node actions (counting, for the memory chart, and extracting, for the breadcrumbs HTML). changes were made to the stepper so it could accept a list of callbacks, and return all types of data in one shot, so we only need one hiddenreplayerinstance on the page for the DOM node functions.unscientifically, it seemed like loading breadcrumbs + memory took the same amount of time as loading the memory chart. (see video below -- as soon as the breadcrumbs tab is done loading, the memory tab is already done).
Screen.Recording.2024-07-19.at.11.49.24.AM.mov
this makes sense because we’re iterating over thousands of frames for the DOM nodes chart, so that’s the bottleneck. this means the breadcrumbs tab loads slower.
this PR:
compared to that is the approach where we have 1 stepper instance for breadcrumbs, and one for DOM node count, each iterating over their own lists (which is what i'm doing in this PR). this approach showed breadcrumb data on the screen about 2x as fast as the approach above. therefore 2 stepper instances on the screen is better for users, especially since breadcrumbs tab is more popular than the memory tab.
the video below demonstrates the memoization in action. once the breadcrumbs or memory tabs are loaded, switching back to them does not cause re-loading, because the results are cached. notice that the breadcrumbs tab loading is not dependent on the loading of the DOM tab, unlike the video above where the breadcrumbs tab has to "wait" for the memory tab. (i'm also on slower wifi in this clip below)
Screen.Recording.2024-07-19.at.3.28.54.PM.mov