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
Conversation
355304c
to
9b09ce8
Compare
@CurryYangxx very elegant approach. With node access in webworkers we have opened up some interesting performance options. |
// @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 |
There was a problem hiding this comment.
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.
9d9d86a
to
5918089
Compare
30e453d
to
07f2fc5
Compare
Now it will create just one worker when component first time render.And use a unique id to distinguish worker message.
Use the unique id for every message, only the result which id match the latested id will be updated.
use a cache object in worker, only reload when ruleset path change |
We still need to save spectralRun IPC because it is used in this action:
|
dc30c47
to
df2cb91
Compare
There was a problem hiding this 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.
@ihexxa Good catch, update more logs and necessary termination |
ef2080b
to
50caca4
Compare
Close #4653 #4241
Move spec lint logic to web worker, can improve both opening file and editing file
Changes:
How to test: