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: improve performance when spec lint [INS-3724] #7374

Merged
merged 20 commits into from May 14, 2024

Conversation

CurryYangxx
Copy link
Member

@CurryYangxx CurryYangxx commented May 6, 2024

Close #4653 #4241

Move spec lint logic to web worker, can improve both opening file and editing file

Changes:

  • move spectral lint login to web worker both in the Design component and router action
    • add dedicate worker file
    • delete all the spectral in main process
  • open electron nodeIntegrationInWorker option
  • add vite worker plugin config so we can use electron require magic in worker
  • wrap code-editor with memo to reduce rerender

How to test:

  • trigger lint multiple times and only the latest task result will be updated
  • use custom spectral ruleset and it it works

@CurryYangxx CurryYangxx marked this pull request as draft May 6, 2024 10:09
@jackkav
Copy link
Contributor

jackkav commented May 7, 2024

@CurryYangxx very elegant approach. With node access in webworkers we have opened up some interesting performance options.

tsconfig.base.json Outdated Show resolved Hide resolved
Comment on lines 240 to 244
// @TODO: Conisderations and things we need to verify/test:
// - Should we spawn a new worker on every lint command or use a shared worker?
// - Does this function handle race conditions? e.g. if a lint finishes late does it replace another lint that ran with latest contents?
// - Proper error handling from the worker and here. Perhaps the worker returns a result type like Result = Diagnostics | Error and we handle it here
// - Don't reload the spectral ruleset on every lint command. Instead the ruleset should be re-calculated when the underlying file changes due to e.g. git pull or the filename changes
Copy link
Contributor

@jackkav jackkav May 7, 2024

Choose a reason for hiding this comment

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

Should we spawn a new worker on every lint command or use a shared worker?

it depends on the error handling story

Does this function handle race conditions?

no if you always spawn a new one, although since it can happen on keystroke we might need to throttle the creation of workers, or resetting a shared one.

Proper error handling from the worker and here.

the simplest way I can imagine is wrapping the worker in a promise and using abort and reject when listening on the worker error event.

Don't reload the spectral ruleset on every lint command
agreed, but perhaps we can consider this is an optimisation for later.

@CurryYangxx CurryYangxx force-pushed the perf/web-worker-lint-spec branch 2 times, most recently from 9d9d86a to 5918089 Compare May 7, 2024 09:08
@CurryYangxx CurryYangxx changed the title perf: improve performance when spec lint perf: improve performance when spec lint [INS-3724] May 7, 2024
@CurryYangxx CurryYangxx force-pushed the perf/web-worker-lint-spec branch 2 times, most recently from 30e453d to 07f2fc5 Compare May 8, 2024 08:49
@CurryYangxx
Copy link
Member Author

CurryYangxx commented May 8, 2024

Should we spawn a new worker on every lint command or use a shared worker?

Now it will create just one worker when component first time render.And use a unique id to distinguish worker message.

Does this function handle race conditions?

Use the unique id for every message, only the result which id match the latested id will be updated.

Don't reload the spectral ruleset on every lint command

use a cache object in worker, only reload when ruleset path change

@CurryYangxx
Copy link
Member Author

We still need to save spectralRun IPC because it is used in this action:

const results = (await window.main.spectralRun({ contents: apiSpec.contents, rulesetPath })).filter(isLintError);

@CurryYangxx CurryYangxx marked this pull request as ready for review May 8, 2024 09:28
@CurryYangxx CurryYangxx requested a review from a team May 8, 2024 10:29
@CurryYangxx CurryYangxx force-pushed the perf/web-worker-lint-spec branch 2 times, most recently from dc30c47 to df2cb91 Compare May 13, 2024 03:09
ihexxa
ihexxa previously approved these changes May 13, 2024
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.

Discussed offline and resolved several concerns, just added some minor comments. Overall it looks good to me but let's wait a bit as there might be some comments from other reviewers.

packages/insomnia/src/ui/routes/actions.tsx Show resolved Hide resolved
packages/insomnia/src/ui/worker/spectral.ts Outdated Show resolved Hide resolved
packages/insomnia/src/ui/worker/spetral-run.ts Outdated Show resolved Hide resolved
@CurryYangxx
Copy link
Member Author

CurryYangxx commented May 13, 2024

Discussed offline and resolved several concerns, just added some minor comments. Overall it looks good to me but let's wait a bit as there might be some comments from other reviewers.

@ihexxa Good catch, update more logs and necessary termination

gatzjames
gatzjames previously approved these changes May 13, 2024
@CurryYangxx CurryYangxx merged commit 03afa4d into develop May 14, 2024
7 checks passed
@CurryYangxx CurryYangxx deleted the perf/web-worker-lint-spec branch May 14, 2024 06:56
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.

Insomnia has become unresponsive open api specs 3
4 participants