Skip to content

Conversation

@apapirovski
Copy link
Contributor

A couple of very slight refinements to process.nextTick & its supporting code.

  • Remove length prop on NextTickQueue class. We technically don't need to keep track of the length of the queue in two places as we already have tickInfo doing that work (between the index & the length we have enough data for everything).
  • Store asyncId in a const within the _tickCallback function. Accessing symbol properties seems to be quite a bit more expensive than string keys so this actually has a decent performance impact.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

process

@apapirovskiapapirovski added the process Issues and PRs related to the process subsystem. label Dec 2, 2017
@addaleaxaddaleax added the performance Issues and PRs related to the performance of Node.js. label Dec 2, 2017
@apapirovski
Copy link
ContributorAuthor

apapirovski commented Dec 2, 2017

apapirovski added a commit that referenced this pull request Dec 6, 2017
Remove length prop on NextTickQueue class. We technically don't need to keep track of the length of the queue in two places as we already have tickInfo doing that work (between the index & the length we have enough data for everything). Store asyncId in a const within the _tickCallback function. Accessing Symbol properties seems to be quite a bit more expensive than string keys so this actually has a decent performance impact. PR-URL: #17421 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@apapirovski
Copy link
ContributorAuthor

Landed in b1acba3

@apapirovskiapapirovski deleted the patch-process-next-tick branch December 6, 2017 18:16
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Remove length prop on NextTickQueue class. We technically don't need to keep track of the length of the queue in two places as we already have tickInfo doing that work (between the index & the length we have enough data for everything). Store asyncId in a const within the _tickCallback function. Accessing Symbol properties seems to be quite a bit more expensive than string keys so this actually has a decent performance impact. PR-URL: #17421 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Remove length prop on NextTickQueue class. We technically don't need to keep track of the length of the queue in two places as we already have tickInfo doing that work (between the index & the length we have enough data for everything). Store asyncId in a const within the _tickCallback function. Accessing Symbol properties seems to be quite a bit more expensive than string keys so this actually has a decent performance impact. PR-URL: #17421 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 12, 2017
@gibfahngibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Dec 20, 2017
@MylesBorins
Copy link
Contributor

Were the perf benefits in this high enough to backport?

@targostargos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performanceIssues and PRs related to the performance of Node.js.processIssues and PRs related to the process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@apapirovski@MylesBorins@jasnell@addaleax@lpinca@targos@BethGriggs@gibfahn