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.

Sign up for GitHub

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

Jump to bottom

Limitted microtasks #530

Closed
wants to merge 3 commits into from
Closed

Limitted microtasks #530

wants to merge 3 commits into from

Conversation

aricart
Copy link
Member

@aricart aricart commented Jun 5, 2023

No description provided.

@aricart aricart requested a review from caspervonb June 5, 2023 18:52
caspervonb
caspervonb approved these changes Jun 6, 2023
View reviewed changes
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

Tested extensively, lgtm (one nit, optional).

src/deno_transport.ts Outdated
}
// if we have max number of pending microtasks
// schedule a timeout
if (this.microtasks === 1000) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Add a const for this, tested up to a million and doesn't fail.

Suggested change
if (this.microtasks === 1000) {
if (this.microtasks === 1000) {

@aricart aricart temporarily deployed to CI June 6, 2023 20:30 — with GitHub Actions Inactive
@aricart aricart temporarily deployed to CI June 6, 2023 20:53 — with GitHub Actions Inactive
@aricart aricart temporarily deployed to CI June 6, 2023 20:57 — with GitHub Actions Inactive
@aricart
Copy link
Member Author

aricart commented Jun 6, 2023

@caspervonb So I added a quick test printing the pending microtasks to see if there was some relation to when deno hits the limit on Uncaught RangeError: Maximum call stack size exceeded - and found something that doesn't make sense to me - never more than one or two microtask is queued - never. The PR is solid in performance, but I want to understand why.

As I read the code, Protocol#sendCommand() is not awaiting, so in a bench loop, it is executing - meaning that the event loop has not yet terminated, meaning that microtasks should be queued (indeed - if you print flush pending - many happen before the first microtask shows), yet this doesn't happen.

@aricart aricart closed this Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caspervonb caspervonb caspervonb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aricart @caspervonb