Optimize Venues search using React Concurrent Mode APIs by taneliang · Pull Request #3040 · nusmodifications/nusmods

taneliang · 2020-12-08T14:51:22Z

TL;DR

Uses React Concurrent Mode APIs (added in #3039) to optimize the Venues components reworked in #3038.

Optimizations in this PR

Uses useDeferredValue to allow the search box to be immediately updated in a sync render before a second, longer concurrent render is performed.

master This PR

This may sound similar to the DIY deferred update using 2 nested requestAnimationFrame calls that we currently have on master. However, that is inferior to this solution because:
1. CM allows in-progress renders to be abandoned, allowing multiple keystrokes in quick-ish succession to remain responsive, as previous in-progress renders are simply abandoned. On master, the app noticeably freezes up once a legacy render starts as it cannot be interrupted. In the gifs above, notice that on master, a bunch of characters appear at once -- this happens because I typed while the app was frozen.
2. (Independent of CM) Overhaul Venues components #3038 reduces unnecessary renders of expensive components, which reduces the amount of time the browser spends frozen.
This is what typing a single character looks like in the React Scheduling Profiler (that I helped create :D):

TL;DR of how to read the React data (the stuff above the grey bar):
- The circles represent state updates in this case.
- Each horizontal row = a React fiber lane (each of which has a purpose/priority)
- A blue box in each lane represents a render chunk, and a red box is a commit phase.
Notice the short sync render on the left (which is in response to the key press), followed by a very long render caused by the deferred value updating. Also notice that VenueList takes a long time to render.

This is what typing a second character during a render looks like:

Notice that a concurrent render got abandoned.
Uses useDeferredValue to allow the availability filter to open instantly. On master, this just freezes as there were no optimizations for this.

Note that we do not also defer searchOptions, as I couldn't get it to feel right. It seems better to just let the page freeze while the render happens. (Edit: it's now deferred. See GIFs in this comment below: #3040 (comment))

codecov · 2020-12-08T14:57:32Z

Codecov Report

Merging #3040 (ca2a38b) into eliang/concurrent-mode (de2dee6) will increase coverage by 0.08%.
The diff coverage is 90.47%.

@@                    Coverage Diff                     @@
##           eliang/concurrent-mode    #3040      +/-   ##
==========================================================
+ Coverage                   57.16%   57.25%   +0.08%     
==========================================================
  Files                         257      258       +1     
  Lines                        5286     5297      +11     
  Branches                     1206     1212       +6     
==========================================================
+ Hits                         3022     3033      +11     
  Misses                       2264     2264

Impacted Files	Coverage Δ
...es/ModuleFinderContainer/ModuleFinderContainer.tsx	`0.00% <0.00%> (ø)`
website/src/views/components/LoadingOverlay.tsx	`100.00% <100.00%> (ø)`
website/src/views/components/SearchBox.tsx	`100.00% <100.00%> (ø)`
website/src/views/venues/VenuesContainer.tsx	`80.00% <100.00%> (+2.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de2dee6...ca2a38b. Read the comment docs.

nusmods-deploy-bot · 2020-12-08T14:59:11Z

Deployment preview for ca2a38bd ready at http://5fdefb7a5687bdde3cef2f5d--nusmods-deploy-preview.netlify.app

taneliang · 2020-12-08T15:06:35Z

+          {isAvailabilityEnabled !== deferredIsAvailabilityEnabled ? (
+            <LoadingSpinner className={styles.availabilitySpinner} white={isAvailabilityEnabled} />


This causes the spinner to flash on faster browsers. No idea how to fix it, but I think storing isAvailabilityEnabled !== deferredIsAvailabilityEnabled in a useState and using our defer may be able to fix it at the cost of more renders?

How do I repro this flashing? I tried to profile it but I couldn't see the flashing

Oh, just click the Find free rooms button. The clock icon in the button will disappear for a moment as the spinner flashes

I'm not sure if putting the spinner here is the best idea - the thing that's "loading" is the venue list below, and it's quite easily missed (though the animation helps). Won't putting a spinner there instead work?

Also thinking about https://reactjs.org/docs/concurrent-mode-patterns.html#delaying-a-pending-indicator, but I don't know how to make that work with useDeferredValue

Ooh, deferring the loading indicator in CSS is brilliant. I'll switch us to useTransition then, especially since the current condition for determining whether to show the indicator or not is a bit of a hack.

I lied, useTransition isn't a good fit for our use case. It's back to useDeferredValue, but with a deferred spinner :D

taneliang · 2020-12-08T15:06:51Z

        <button
          className={classnames(
            'btn btn-block btn-svg',
-            styles.availabilityToggle,


This style didn't exist

taneliang · 2020-12-18T08:59:39Z

Rebased on parent branch, and fixed conflict with d5b270e (#3038)

taneliang · 2020-12-18T12:55:43Z

Main changes:

Spinner is now overlaid on VenueList
Spinner and overlay are now deferred by 0.2s. The experience is far better.
searchOptions is now deferred -- selecting stuff in the Find free rooms is now super smooth 😍

Updated gifs:

No throttling	6x CPU throttling

…reezing

…ary and deferred values manually

taneliang · 2020-12-19T09:49:20Z

Rebased on parent branch, no diff change

ZhangYiJiang

I'm going to need a bit more time reading up the concurrent mode docs to understand what useDeferredValue is doing here

ZhangYiJiang · 2020-12-19T20:13:09Z

+// Source: https://reactjs.org/docs/concurrent-mode-patterns.html#delaying-a-pending-indicator
+.deferred {
+  visibility: hidden;
+  animation: 0s linear 0.2s forwards makeVisible;


We could in theory make this configurable from the component side using CSS variables or even inline styles, but probably YAGNI

Yeah, probably not a good idea to overengineer this right now hahaa

ZhangYiJiang · 2020-12-19T20:13:51Z

  placeholder?: string;
  onChange: (value: string) => void;
-  onSearch: () => void;
+  onSearch?: () => void;


Might be useful adding a small comment on the difference between this and onChange

ZhangYiJiang · 2020-12-19T20:18:18Z

            />

            <LoadingComponent>
-              <div className={styles.loadingOverlay} />


Should remove the unused duplicate class here from ModuleFinderContainer.scss

Yep, it was already removed

ZhangYiJiang · 2020-12-19T20:19:26Z

-  ] = useState<string>(() => qs.parse(location.search).q || '');
-  /** Actual string to search with; deferred update */
-  const deferredSearchQuery = searchQuery; // TODO: Redundant now. Use React.useDeferredValue after we adopt concurrent mode
+  const [searchQuery, setSearchQuery] = useState<string>(() => qs.parse(location.search).q || '');


/nit

Suggested change

const [searchQuery, setSearchQuery] = useState<string>(() => qs.parse(location.search).q || '');

const [searchQuery, setSearchQuery] = useState(() => qs.parse(location.search).q || '');

That doesn't work because qs.parse returns any. We could do the below type assertion instead, but the current code seems good enough

Suggested change

const [searchQuery, setSearchQuery] = useState<string>(() => qs.parse(location.search).q || '');

const [searchQuery, setSearchQuery] = useState(

() => (qs.parse(location.search) as Params).q || '',

);

ZhangYiJiang · 2020-12-19T20:19:44Z

+  const handleSearchChange = useCallback((newSearchQuery: string) => {
+    setSearchQuery(newSearchQuery);
+  }, []);


Isn't handleSearchChange exactly the same as setSearchQuery?

Oh yes, I think I forgot to change this back after my brief experiment with useTransition

ZhangYiJiang · 2020-12-19T20:28:54Z

-        {size(matchedVenues) === 0 ? (
-          renderNoResult()
-        ) : (
+        <div className="position-relative">


I think without specifying height this will not take up the whole height of the venues list right?

I think it does since it wraps the venue list component

taneliang · 2020-12-20T06:29:01Z

@ZhangYiJiang I think reading useDeferredValue's implementation may be easiest. https://github.com/facebook/react/blob/99554dc36fa9f5ef29b75dd59836dcc720d831b9/packages/react-reconciler/src/ReactFiberHooks.old.js#L1498-L1538

The mount* functions are called the first time the hook is called, and the update* functions are called on subsequent calls. The *State and *Effect functions correspond to useState and useEffect.

TL;DR it looks like it's just storing the previous state in a useState, then doing a state update in a passive effect (you can also see this happening with the scheduling profiler). But interestingly it also sets a transition flag on the fiber (which is now just 1 or 0, but in the past it was the suspense config object that you see in the CM docs). I'm not sure yet what transition accomplishes, but useTransition does that as well.

taneliang mentioned this pull request Dec 8, 2020

Overhaul Venues components #3038

Open

taneliang commented Dec 8, 2020

View reviewed changes

taneliang marked this pull request as ready for review December 8, 2020 15:11

taneliang requested a review from a team December 8, 2020 15:11

taneliang mentioned this pull request Dec 8, 2020

Adopt React concurrent mode #3039

Closed

taneliang force-pushed the eliang/concurrent-mode branch from 4506e67 to 156c1e3 Compare December 18, 2020 08:55

taneliang force-pushed the eliang/optimize-venue-search-box branch from c8201a6 to 07601ec Compare December 18, 2020 08:58

taneliang force-pushed the eliang/concurrent-mode branch from 156c1e3 to de2dee6 Compare December 19, 2020 09:48

taneliang added 6 commits December 19, 2020 17:49

Improve venue search box responsiveness

439bfcb

useDeferredValue(isAvailabilityEnabled) with spinner to reduce page f…

bb8b6af

…reezing

Move spinner to an overlay

a6d0cab

Reduce defer delay to improve perceived responsiveness

80102e5

Defer searchOptions

4423a00

Switch back to useDeferredValue as it's too messy to manage both prim…

ce167bc

…ary and deferred values manually

taneliang force-pushed the eliang/optimize-venue-search-box branch from 26d33b3 to ce167bc Compare December 19, 2020 09:49

taneliang added performance venues page labels Dec 19, 2020

ZhangYiJiang reviewed Dec 19, 2020

View reviewed changes

taneliang added 3 commits December 20, 2020 14:37

Fix review comment: add onSearch and onChange docstrings

391e96b

Fix review comment: remove unnecessary handleSearchChange

e596bc5

Make SearchBox throttle prop optional

ca2a38b

		{isAvailabilityEnabled !== deferredIsAvailabilityEnabled ? (
		<LoadingSpinner className={styles.availabilitySpinner} white={isAvailabilityEnabled} />

	const [searchQuery, setSearchQuery] = useState<string>(() => qs.parse(location.search).q \|\| '');
	const [searchQuery, setSearchQuery] = useState(() => qs.parse(location.search).q \|\| '');

-  const [searchQuery, setSearchQuery] = useState<string>(() => qs.parse(location.search).q || '');
+  const [searchQuery, setSearchQuery] = useState(
+    () => (qs.parse(location.search) as Params).q || '',
+  );

Conversation

taneliang commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Optimizations in this PR

Uh oh!

codecov Bot commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nusmods-deploy-bot Bot commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taneliang Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

taneliang commented Dec 18, 2020

Uh oh!

taneliang commented Dec 18, 2020

Uh oh!

taneliang commented Dec 19, 2020

Uh oh!

ZhangYiJiang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

taneliang commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

taneliang commented Dec 8, 2020 •

edited

Loading

codecov Bot commented Dec 8, 2020 •

edited

Loading

nusmods-deploy-bot Bot commented Dec 8, 2020 •

edited

Loading

taneliang Dec 8, 2020 •

edited

Loading

ZhangYiJiang left a comment •

edited

Loading

taneliang commented Dec 20, 2020 •

edited

Loading