perf: App start improvement [INS-3957] #7492

CurryYangxx · 2024-06-05T09:11:00Z

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 · 2024-06-11T03:39:32Z

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

packages/insomnia/src/ui/hooks/use-organization-async-task.ts

jackkav · 2024-06-12T07:59:15Z

packages/insomnia/src/ui/routes/organization.tsx

+  }
+};
+
+async function migrateProjectsUnderOrganization(personalOrganizationId: string, sessionId: string) {


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

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 · 2024-06-12T08:02:55Z

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 · 2024-06-12T09:46:23Z

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.

packages/insomnia/src/utils/router.ts

jackkav · 2024-06-13T06:08:44Z

packages/insomnia/src/ui/routes/organization.tsx

+    for (const task of asyncTaskList) {
+      if (task === AsyncTask.SyncOrganization) {
+        invariant(sessionId, 'sessionId is required');
+        invariant(accountId, 'accountId is required');
+        taskPromiseList.push(syncOrganization(sessionId, accountId));
+      }
+
+      if (task === AsyncTask.MigrateProjects) {
+        invariant(personalOrganizationId, 'personalOrganizationId is required');
+        invariant(sessionId, 'sessionId is required');
+        taskPromiseList.push(migrateProjectsUnderOrganization(personalOrganizationId, sessionId));
+      }
+
+      if (task === AsyncTask.SyncProjects) {
+        invariant(organizationId, 'organizationId is required');
+        taskPromiseList.push(syncProjects(organizationId));
+      }
+    }
+
+    await Promise.all(taskPromiseList);


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.

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

CurryYangxx · 2024-06-21T06:20:25Z

packages/insomnia/src/ui/routes/organization.tsx

+  useEffect(() => {
+    // sync organizations
+    if (asyncTaskList.includes(AsyncTask.SyncOrganization)) {
+      console.log('running sync organization task');
+      const submit = syncOrgsFetcher.submit;
+      submit({}, {
+        action: '/organization/sync',
+        method: 'POST',
+      });
+    }
+  }, [asyncTaskList, syncOrgsFetcher.submit]);
+
+  useEffect(() => {
+    // sync projects under organization
+    if (asyncTaskList.includes(AsyncTask.SyncProjects)) {
+      console.log('running sync projects task');
+      const submit = syncProjectsFetcher.submit;
+      submit({}, {
+        action: `/organization/${organizationId}/sync-projects`,
+        method: 'POST',
+      });
+    }
+  }, [asyncTaskList, organizationId, syncProjectsFetcher.submit]);
+
+  useEffect(() => {
+    // migrate projects under personal organization
+    if (asyncTaskList.includes(AsyncTask.MigrateProjects)) {
+      console.log('running migrate projects task');
+      const submit = migrateProjectsFetcher.submit;
+      submit({}, {
+        action: '/organization/migrate-projects',
+        method: 'POST',
+      });
+    }
+  }, [asyncTaskList, migrateProjectsFetcher.submit]);
+


@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.

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 · 2024-06-25T13:37:57Z

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 · 2024-06-25T15:39:02Z

packages/insomnia/src/ui/routes/organization.tsx

+  // 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


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.

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.

I think this idea is not feasible for two reasons

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.

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 .

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 · 2024-06-17T07:47:17Z

packages/insomnia/src/models/organization.ts

+export const findPersonalOrganization = (organizations: Organization[], accountId: string) => {
+  return organizations.filter(isPersonalOrganization)
+    .find(organization =>
+      isOwnerOfOrganization({


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

packages/insomnia/src/utils/router.ts

ihexxa · 2024-07-01T09:16:34Z

packages/insomnia/src/ui/routes/organization.tsx

+  // 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


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!

packages/insomnia/src/ui/routes/organization.tsx

packages/insomnia/src/ui/routes/project.tsx

packages/insomnia/src/utils/router.ts

packages/insomnia/src/ui/routes/project.tsx

jackkav · 2024-07-05T11:11:48Z

packages/insomnia/src/utils/router.ts

+  SyncOrganization,
+  MigrateProjects,
+  SyncProjects,
+}


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

Good advice. Added to my todo list

CurryYangxx marked this pull request as draft June 5, 2024 09:11

CurryYangxx changed the title ~~wip: gen whole route in initialEntry~~ wip: gen whole route in initialEntry [INS-3957] Jun 5, 2024

CurryYangxx changed the title ~~wip: gen whole route in initialEntry [INS-3957]~~ perf: gen whole route in initialEntry [INS-3957] Jun 11, 2024

CurryYangxx marked this pull request as ready for review June 11, 2024 03:39

CurryYangxx requested a review from a team June 11, 2024 03:40

CurryYangxx mentioned this pull request Jun 11, 2024

feat: add async task indicator [INS-4106] #7522

Merged

2 tasks

jackkav reviewed Jun 12, 2024

View reviewed changes

packages/insomnia/src/ui/hooks/use-organization-async-task.ts Outdated Show resolved Hide resolved

jackkav reviewed Jun 12, 2024

View reviewed changes

CurryYangxx changed the title ~~perf: gen whole route in initialEntry [INS-3957]~~ perf: App start improvement - gen whole route in initialEntry [INS-3957] Jun 12, 2024

jackkav reviewed Jun 13, 2024

View reviewed changes

packages/insomnia/src/utils/router.ts Outdated Show resolved Hide resolved

jackkav reviewed Jun 13, 2024

View reviewed changes

CurryYangxx requested review from gatzjames and ihexxa June 14, 2024 07:57

CurryYangxx changed the title ~~perf: App start improvement - gen whole route in initialEntry [INS-3957]~~ perf: App start improvement [INS-3957] Jun 19, 2024

CurryYangxx changed the base branch from develop to release-performance-improvement June 21, 2024 02:59

CurryYangxx commented Jun 21, 2024

View reviewed changes

CurryYangxx changed the base branch from release-performance-improvement to develop June 24, 2024 03:29

CurryYangxx requested a review from jackkav June 24, 2024 05:49

CurryYangxx force-pushed the perf/modify-initial-entry branch from 39cc512 to f364d71 Compare June 24, 2024 06:15

filfreire reviewed Jun 25, 2024

View reviewed changes

jackkav reviewed Jun 25, 2024

View reviewed changes

ihexxa reviewed Jul 1, 2024

View reviewed changes

CurryYangxx force-pushed the perf/modify-initial-entry branch from 12aa93d to 55b5f6c Compare July 5, 2024 06:26

CurryYangxx added 3 commits July 5, 2024 14:33

wip: gen whole route in initialEntry

e6f1392

async tasks

8b422cc

add params

77e093d

CurryYangxx and others added 6 commits July 5, 2024 14:33

add comments

dcfe687

refactor

3875906

fix await

d7348d6

feddback

a81978a

return string type

f18b525

perf: return deferredData in project loader

4cc27bc

CurryYangxx force-pushed the perf/modify-initial-entry branch from 55b5f6c to 4cc27bc Compare July 5, 2024 06:36

clean code

dc79939

gatzjames reviewed Jul 5, 2024

View reviewed changes

jackkav reviewed Jul 5, 2024

View reviewed changes

jackkav previously approved these changes Jul 5, 2024

View reviewed changes

gatzjames previously approved these changes Jul 5, 2024

View reviewed changes

modify var name

76bdb31

CurryYangxx dismissed stale reviews from gatzjames and jackkav via 76bdb31 July 8, 2024 06:17

ihexxa previously approved these changes Jul 8, 2024

View reviewed changes

CurryYangxx enabled auto-merge (squash) July 8, 2024 06:21

chore: add git sync enable flag for test

72513b9

CurryYangxx dismissed ihexxa’s stale review via 72513b9 July 8, 2024 08:02

ihexxa approved these changes Jul 8, 2024

View reviewed changes

CurryYangxx merged commit deacf15 into develop Jul 8, 2024
7 checks passed

CurryYangxx deleted the perf/modify-initial-entry branch July 8, 2024 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: App start improvement [INS-3957] #7492

perf: App start improvement [INS-3957] #7492

CurryYangxx commented Jun 5, 2024 •

edited by gatzjames

Loading

CurryYangxx commented Jun 11, 2024

jackkav Jun 12, 2024

CurryYangxx Jun 12, 2024 •

edited

Loading

jackkav commented Jun 12, 2024

CurryYangxx commented Jun 12, 2024

jackkav Jun 13, 2024 •

edited

Loading

CurryYangxx Jun 13, 2024

CurryYangxx Jun 21, 2024 •

edited

Loading

CurryYangxx Jun 24, 2024 •

edited

Loading

filfreire left a comment

CurryYangxx commented Jun 25, 2024

jackkav Jun 25, 2024

jackkav Jun 25, 2024

CurryYangxx Jun 26, 2024 •

edited

Loading

ihexxa Jul 1, 2024

ihexxa Jun 17, 2024

ihexxa Jul 1, 2024

gatzjames left a comment

jackkav Jul 5, 2024 •

edited

Loading

CurryYangxx Jul 8, 2024

perf: App start improvement [INS-3957] #7492

perf: App start improvement [INS-3957] #7492

Conversation

CurryYangxx commented Jun 5, 2024 • edited by gatzjames Loading

Changes

Loading strategy

return deferred value in project loader

Test

CurryYangxx commented Jun 11, 2024

Choose a reason for hiding this comment

CurryYangxx Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

jackkav commented Jun 12, 2024

CurryYangxx commented Jun 12, 2024

jackkav Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CurryYangxx Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

CurryYangxx Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

filfreire left a comment

Choose a reason for hiding this comment

CurryYangxx commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CurryYangxx Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatzjames left a comment

Choose a reason for hiding this comment

jackkav Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CurryYangxx commented Jun 5, 2024 •

edited by gatzjames

Loading

CurryYangxx Jun 12, 2024 •

edited

Loading

jackkav Jun 13, 2024 •

edited

Loading

CurryYangxx Jun 21, 2024 •

edited

Loading

CurryYangxx Jun 24, 2024 •

edited

Loading

CurryYangxx Jun 26, 2024 •

edited

Loading

jackkav Jul 5, 2024 •

edited

Loading