Skip to content

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