falling-fruit-web
falling-fruit-web copied to clipboard
falling-fruit
Performance
Description
Panning and zooming the Google Map component is often sluggish (in a way that the current website is not). Loading the type filter is slow, and impacts the performance of the map.
Possible interventions
- General
- [ ] https://github.com/falling-fruit/falling-fruit-web/issues/93
- Google Maps
- [ ] https://github.com/falling-fruit/falling-fruit-web/issues/142
- Type filter
- [ ] Use web worker to improve type filter performance @SirajChokshi
- [ ] Update
rc-tree-select
Useful links
- https://reactjs.org/docs/optimizing-performance.html
- https://www.infoq.com/articles/reduce-react-load-time/
The main issue is that the API call is made after zoom settles. Find a way to re-order instructions if a "bubble" is clicked. For manual scrolling zoom, I think the existing order of instructions cannot be changed.
@abehara2, this is a great observation. There's two parts to this. Sometimes we can catch the event on zoom before the animation occurs because it's a cluster click. But is it possible for us to have onViewChange called in general before the animation occurs?
Speeding up the app's redrawing of type filters etc. will still help, but I noticed there's an improvement from using the map's native API for e.g. handling cluster click, commit 55dfda3e6d51a7b5974d2fe460e5d8935e391cfb. Calling mapRef.current.panTo and mapRef.current.setZoom does the animations first, so the user gets immediate feedback.
@wbazant @Qrtn The map is sluggish enough to elevate this to high priority. Looking at network traffic while trying to pan the map on a throttled internet connection, I observe that:
On the desktop map, with clusters, the map can be panned without lag if I do it quickly while the /types/counts and /clusters API calls are both still pending (doing so triggers another pair of API calls). Once the /types/counts call is returned, lag is felt. This issue disappears if the type tree is hidden, for example by displaying settings or by switching to the mobile map.
Conclusion: The problem is updating the type tree. But what is remarkable is that the problem persists even if /types/counts returns no counts (such that the tree is not rendered), and even if "On the map" is unchecked (such that the entire type tree is already rendered).
With some console.log sprinkled in, I see that RCTreeSelect is visited 5 times over the course of 2.5 seconds if I pan the map, regardless of how many type counts are returned. Only the last time seems to result in a visible change. constructTypesTreeForSelection is visited only once, roughly towards the end. Building treeNodes within this function seems to take 1 ms.
Any suspicions about what might be going on here?
That narrows it down! I'm guessing that maybe the package does something slow - we give it a list of nodes and it produces a tree, so it likely makes some internal implementation of the nodes. It's not necessarily the package code being bad, but it's probably meant to be general, and might now have been written for how we're using it, which involves changing the list a lot.
The solution could just involve using the app's state indicators about map loading and type counts loading to define a logic that will only rerender once we're done panning and fetching (maybe through redux or telling react to only update when isLoading is false).
Then we could hand-write the code currently provided by the library, so it does just what we want, and if it's still slow, try to optimize that:
- tweak constructTypesTreeForSelection output: instead of a flat list, use a nested structure with recursion, etc.
- come up with an id / props scheme that we can rely on not to rerender
@wbazant The numbers I gave above are for the #419 branch, which felt to me especially sluggish. On the main branch, RCTreeSelect is visited 2 times (versus 5) over the course of 0.5 seconds if I pan the map. So somehow the branch made things worse?!
Before we consider writing our own nested tree, shouldn't we first try to prevent the tree internals from blocking the map?
I think I reproduced your conclusion today, these logs were helpful in RCTreeSelect:
<<<<<<< SEARCH
const RCTreeSelect = ({ data, onChange, types, searchValue }) => {
// useState is necessary instead of useRef in order to restore the container ref whenever the tree re-renders
const [treeSelectContainerRef, setTreeSelectContainerRef] = useState(null)
=======
const RCTreeSelect = ({ data, onChange, types, searchValue }) => {
console.log('RCTreeSelect rendering');
// useState is necessary instead of useRef in order to restore the container ref whenever the tree re-renders
const [treeSelectContainerRef, setTreeSelectContainerRef] = useState(null)
useEffect(() => {
const startTime = performance.now();
return () => {
const endTime = performance.now();
console.log(`RCTreeSelect render time: ${endTime - startTime} ms`);
};
});
>>>>>>> REPLACE
The times are like:
RCTreeSelect.js:123 RCTreeSelect render time: 1238.9000000357628 ms
RCTreeSelect.js:123 RCTreeSelect render time: 284.30000001192093 ms
RCTreeSelect.js:123 RCTreeSelect render time: 293.5 ms
RCTreeSelect.js:123 RCTreeSelect render time: 628.1000000238419 ms
RCTreeSelect.js:123 RCTreeSelect render time: 295.39999997615814 ms
RCTreeSelect.js:123 RCTreeSelect render time: 686 ms
RCTreeSelect.js:123 RCTreeSelect render time: 282.30000001192093 ms
RCTreeSelect.js:123 RCTreeSelect render time: 290.2999999523163 ms
RCTreeSelect.js:123 RCTreeSelect render time: 562.1999999880791 ms
RCTreeSelect.js:123 RCTreeSelect render time: 307.10000002384186 ms
and this is for an empty place of the map with no checkboxes on it (so no DOM operations are actually being done for this render).
I feel like your point about double /types load in https://github.com/falling-fruit/falling-fruit-web/pull/419#issuecomment-2293199954 is a second clue. This is because I've not used the new class for types access in filters - I've meant to, but once I got the types access change to a place where the app started rendering again, I got distracted an I started doing something else.
shouldn't we first try to prevent the tree internals from blocking the map?
How would you do that? If TreeSelect does something computationally intensive for 0.3 seconds (my hypothesis) I think it just slows down everything on the page until it finishes.
I'm also realising we're using an older version of rc-tree-select. I tried to bump the version just now but it didn't work straight away. Also, it's quite a popular package, and provides many other ways of providing data to it - so we'll probably be able to use it successfully instead of rewriting.
I think the next thing I want to do is improve the filter redux slice and adjacent code - it shouldn't use state.misc.typesById, and I should understand what it does after a view change - and maybe I'll get some ideas on the performance issue while I'm on it.
It's definitely the type tree, because after I stub it out:
return "stubbed" || (
<TreeSelectContainer
ref={setTreeSelectContainerRef}
isSearching={searchValue !== ''}
>
the map becomes enjoyably snappy. There are five renders after a map pan, with this sort of pattern:
RCTreeSelect.js:123 RCTreeSelect render time: <time since the last pan, can be many seconds / minutes>
RCTreeSelect.js:123 RCTreeSelect render time: 7.599999964237213 ms
RCTreeSelect.js:123 RCTreeSelect render time: 18.100000023841858 ms
RCTreeSelect.js:123 RCTreeSelect render time: 622.6999999880791 ms
RCTreeSelect.js:123 RCTreeSelect render time: 11.699999988079071 ms
Adding a useMemo in https://github.com/falling-fruit/falling-fruit-web/pull/419/commits/92240994b43e57b00c5628f171d85b91be13fa8e improves things - the map navigation is only held back some of the cycle - and the timing pattern is the same as with the stubbed component (five logs and only one is long). It's still a bit crappy but feels like progress!
Panning and zooming the Google Map component is now very fast after writing a custom type filter.