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

lib: use deferred queue for all internal ticks calls #51279

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 24, 2023

Refs: #51267

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http
  • @nodejs/http2
  • @nodejs/net
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 24, 2023
@Qard
Copy link
Member

Qard commented Dec 27, 2023

Might need to be semver-major as the timing change could be observable and therefore may be depended on by some users.

@ronag ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 27, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I have a feeling this would be a significant negative impact on performance, but I would love to be proven wrong.

A couple of notes:

  1. I would keep _deferTick private, and not attached to process
  2. a CITGM run is necessary, but we would also need more modules in CITGM

@ronag
Copy link
Member Author

ronag commented Dec 27, 2023

I have a feeling this would be a significant negative impact on performance, but I would love to be proven wrong.

What's your reasoning? Is the existing benchmark suite sufficient to prove you wrong?

@mcollina
Copy link
Member

There is a lot more code in one of hottest functions of core

@mcollina
Copy link
Member

The existing benchmarks would do, but we should check most of yhe affected subsystems.

@ShogunPanda
Copy link
Contributor

I second the request to make the deferTick method private initially.
Unless we figure out the real impact internally I would avoid also having to deal with user code for now.

@ronag ronag closed this Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants