perf: App start improvement [INS-3957]

[CurryYangxx]

close INS-4026 INS-3957
objective: Reduce the App start duration

Changes

Loading strategy

• use the findBestRoute function to try to return the path with orgId and projectId in:
  • initialEntry
  • organizationId indexLoader
• sync task: submit action in useEffect instead of syncing in index route loader (organization index & organizationId index)

return deferred value in project loader

This solution is beneficial for both app start and projects change.

• return deferred data in project loader and wait promise in component

Test

• user has no cache data
• user has cache data

Other approaches we tried:

• Use the router navigation events:
  • Added more complexity on the routing side and relies on private router APIs
  • Simplifies the useEffect logic
  • Adds another way to fetch and store data and caching still needs another solution
  • router.subscribe executes many times in one navigation, we need use a flag to control execution times
  • There's uncertainty about the execution timing (e.g. has the component rendered before we fetch the data or after?)
  • Having a status of which data has been fetched is still hard to achieve
• Use a cache and move the fetching to loaders using defer as a mechanism to update
  • Better UX and DX
  • When using defer we have a mechanism to show stale data on the UI and update when fresh data come in.
  • The problem that remains is cache invalidation and fetching too many times since fetching now happens on the hot path of the app

[CurryYangxx]

have some console.log for testing now, will be deleted before merge

[jackkav]

its really dangerous to mess with migrations, I'd scope this out if you can without introducing more risk

[CurryYangxx]

The origin migration logic is in /organizationId index route loader, after the initial entry change, the user will not match this index route, so we need to put the migration in action.If we could add some test to reduce the risk?

[jackkav]

can you provide more evidence that this approach is addressing the lionshare of performance impact? from what i can see this PR is doing at least two things. in a perf PR please only focus on a single measurable improvement and give a rationale about why this perf change was chosen in context of other perf impacts which were also investigated.

eg.

thing a 2%
thing b 10%
thing c 88%
i chose c because its was worth optimizing.

[CurryYangxx]

can you provide more evidence that this approach is addressing the lionshare of performance impact? from what i can see this PR is doing at least two things. in a perf PR please only focus on a single measurable improvement and give a rationale about why this perf change was chosen in context of other perf impacts which were also investigated.

eg.

thing a 2%
thing b 10%
thing c 88%
i chose c because its was worth optimizing.

Do you mean the two things is modity initial entry and add async task hooks? This is because if we only modify the initial entry, the logic in the organization and organizationId index route loader will not run, so I put the two changes together to make logic of the app be complete.

[jackkav]

Using signals with actions is a new concept. I still don't understand the benefit of this over creating 3 separate actions. This kind of out of the box thinking is cool but really hard to reason about. I don't want it in our happy path if I can help it.

[CurryYangxx]

One reason is 3 seperate action will case loader rerun 3 times.So I merge into only action

[CurryYangxx]

@gatzjames @jackkav if we fetch the existing action instead of combine to a new action, there are 3 action submit, and the loader will re-run 3 times.This will result in repeated requests in the project loader several times.I'd suggest merging into a new action to reduce the number of requests.

[CurryYangxx]

using defer also can not cancel the fetch request in loader

[filfreire]

Some tests on my machine:

Latest beta packaged version:

• 956 ms loading screen until we see dashboard
• 1633 ms activity until fully loaded (affected by network loading of collections and status)

This PR (using packaged app, from npm run app-package):

• 428 ms loading screen until we see dashboard
• 1500 ms activity until fully loaded (affected by network loading of collections and status)

Loading time difference is noticeable.

Switching between Organizations also is fast, but there's a split second when it still is not showing all the unsynced collections right after going to another project/organization.

[CurryYangxx]

Switching between Organizations also is fast, but there's a split second when it still is not showing all the unsynced collections right after going to another project/organization.

@filfreire Thank you very much for your test.Not showing all the unsynced files right away is as expected, because we return a deferred data in react-router loader and wait the promise in component render phase.

[jackkav]

Suggested change

	// ideas:
	// 1. useEffect with controlled execution
	// 2. use fetcher history state to avoid running the same task multiple times
	// 3. create an event after logged in user is detected in initial entry and listen.once in this component
	// ideas:
	// 1. useEffect with controlled execution
	// 2. use fetcher history state to avoid running the same task multiple times
	// 3. create an event after logged in user is detected in initial entry and listen.once in this component
	// 4. rather than use an action hack to avoid fetching in the loader, we can now use a deferred promise in the loader since they are now non-blocking.

[jackkav]

Try this:

// 4. rather than use an action hack to avoid fetching in the loader, we can now use a deferred promise in the loader since they are now non-blocking.

[CurryYangxx]

I think this idea is not feasible for two reasons

1. Because in current solution, we find best route in InitialEntry, and will skip the index route loader, so we are not able to keep fetching in the index route loader.
2. If we don't find best route and keep using '/organization' as the initial entry, the logic is executed to index route loader. But the index route loader just use for refresh cache data and return a redirect, we can't return a deferred promise because no component connected to the loader .

[ihexxa]

The async loading strategy of workspaces seems straightforward with low maintaining effort and which can work with organizations, projects loading in parallel. So I propose to move forward with this part at first.

[ihexxa]

nit: Probably need double check if It is good enough to say that it is a personal org.

[ihexxa]

The async loading strategy of workspaces seems straightforward with low maintaining effort and which can work with organizations, projects loading in parallel. So I propose to move forward with this part at first.

[gatzjames]

Great investigation on the pros/cons of different approaches to improve data fetching.
Let's clean up the code and get this merged!

[jackkav]

consider writing a unit test together for this? to help search for simplifications

[CurryYangxx]

Good advice. Added to my todo list