Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: change org performance improvement [INS-3968] #7582

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

CurryYangxx
Copy link
Member

@CurryYangxx CurryYangxx commented Jun 25, 2024

Changes

  • generate whole route(with orgId and projectId) when click organization icon
  • pass the sync project task into location state, and execute sync project through action
  • when switch to a new organization first time, we can't get projectId from db, so we need redirect route with projectId in syncOrgsAndProjectsAction after sync project.

Spike
May be we can try to sync project in advance when app is idle

@CurryYangxx CurryYangxx marked this pull request as draft June 25, 2024 03:06
@CurryYangxx CurryYangxx changed the title WIP: change org performance improvement perf: change org performance improvement [INS-3968] Jul 2, 2024
@CurryYangxx CurryYangxx force-pushed the perf/change-org branch 2 times, most recently from 6e4455a to 63869df Compare July 9, 2024 06:25
@CurryYangxx CurryYangxx marked this pull request as ready for review July 9, 2024 06:30
@CurryYangxx CurryYangxx requested a review from a team July 9, 2024 06:30
Copy link
Contributor

@ihexxa ihexxa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added several minor comments.

}
to={`/organization/${organization.id}`}
}`}
onClick={async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we should calculate route and async tasks in switching org, do we need to support CTRL+P?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems CTRL+P already get the whole route. So we dont't need do it here.

taskPromiseList.push(syncOrganizations(sessionId, accountId));
}

if (asyncTaskList.includes(AsyncTask.MigrateProjects)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just would like to double check, should we always check and migrate projects before SyncProjects?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

migrate projects only need to be execute in initial entry

@CurryYangxx CurryYangxx force-pushed the perf/change-org branch 3 times, most recently from 101c7ee to c3bb78a Compare July 15, 2024 07:02
@CurryYangxx CurryYangxx requested a review from ihexxa July 15, 2024 07:36
Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CurryYangxx initial tests I did show a 100 to 150 ms improvement from the click event until we draw another organization.

There's a couple of points I think could help the performance improvement:

  • Whenever we switch between Orgs, we have some duplicate API requests being done:
image
  • Reclicking on the exact same organization sends some requests which might not be necessary:
image

@CurryYangxx
Copy link
Member Author

CurryYangxx commented Jul 15, 2024

@CurryYangxx initial tests I did show a 100 to 150 ms improvement from the click event until we draw another organization.

There's a couple of points I think could help the performance improvement:

  • Whenever we switch between Orgs, we have some duplicate API requests being done:

@filfreire Thanks for your test. Duplicate API requests are due to the remix's revalidate mechanism. I will start working on reducing the repeat requests next step.

@CurryYangxx CurryYangxx merged commit 46e2a79 into develop Jul 16, 2024
8 checks passed
@CurryYangxx CurryYangxx deleted the perf/change-org branch July 16, 2024 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants