-
Notifications
You must be signed in to change notification settings - Fork 252
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
Optimize typing a single character #1233
base: main
Are you sure you want to change the base?
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
The initial Could you verify that resolving the promise on a new event loop step carries that performance hit? |
export async function wait(config: Instance['config']): Promise<void> { | ||
const delay = config.delay | ||
if (typeof delay !== 'number') { | ||
return | ||
} | ||
return Promise.all([ | ||
await Promise.all([ |
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.
This makes the function return Promise<undefined>
instead of undefined
for delay: null
and results in await wait()
to add a new micro task.
Are we sure this has no effect?
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.
Doesn't await wait()
always create a new microtask regardless? Doesn't matter what wait()
returns, right?
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.
I'm not sure. I guess it depends on implementation.
Typescript reports 'await' has no effect on the type of this expression. ts(80007)
if you try await undefined
.
The following results in sync
in node@10
and async
in node@12
:
async function waitAsync() {
return undefined
}
function waitSync() {
return undefined
}
(async function () {
await Promise.race([
(async () => {
await waitAsync()
return 'async'
})(),
(async () => {
await waitSync()
return 'sync'
})(),
]).then(console.log)
})()
If we're not sure that this has no effect on any platform that is still supported, I wouldn't touch it if I don't have to.
What:
This improves performance of typing a single character by ~2x
Why:
Users complain about performance #577. Also related to this thread
How:
The desired behavior is to (I suppose) yield to the next macrotask in between keyboard actions. We can avoid the initial
wait
. Some quick benchmarking suggested thosewait
calls make up the bulk of the runtime of this function. When there is only 1 action to run, this brings thewait
calls from 2 => 1Checklist:
I don't believe documentation/tests would be required here.