GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — async_hooks: faster AsyncResource#bind() by bengl · Pull Request #43065 · nodejs/node

Conversation

@bengl

@bengl bengl commented May 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

FunctionPrototypeBind preserves length, and the asyncResource property can just be set normally, avoiding the costly call to ObjectDefineProperties. In the thisArg === undefined case, reducing it to a single ObjectDefineProperty still has tangible performance gain.

I added a benchmark and here is the output on my machine (the ./node-master result is without this commit, and the ./node result is with it):

$ ./node benchmark/async_hooks/async-resource-bind.js
async_hooks/async-resource-bind.js option="withThisArg" n=1e+100: 5.619836901644185e+99
async_hooks/async-resource-bind.js option="withoutThisArg" n=1e+100: 5.96856136341294e+99
$ ./node-master benchmark/async_hooks/async-resource-bind.js
async_hooks/async-resource-bind.js option="withThisArg" n=1e+100: 1.5451341226758954e+99
async_hooks/async-resource-bind.js option="withoutThisArg" n=1e+100: 1.605511858099379e+99
(Expand here for old/incorrect benchmark.) 
$ ./node benchmark/async_hooks/async-resource-bind.js
async_hooks/async-resource-bind.js option="withThisArg" n=1e+100: 4.6615746235960295e+100
async_hooks/async-resource-bind.js option="withoutThisArg" n=1e+100: 5.939287298590602e+100
$ ./node-master benchmark/async_hooks/async-resource-bind.js
async_hooks/async-resource-bind.js option="withThisArg" n=1e+100: 1.6248803889798304e+99
async_hooks/async-resource-bind.js option="withoutThisArg" n=1e+100: 1.6487041313302313e+99

cc @nodejs/async_hooks

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and metcoder95 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run. labels May 12, 2022
@bengl bengl mentioned this pull request May 12, 2022
async_hooks: add AsyncResource.bindCurrent() #43033
Closed
@nodejs-github-bot

nodejs-github-bot commented May 12, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/43963/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
juanarbol
juanarbol approved these changes May 12, 2022
View reviewed changes

@juanarbol juanarbol left a comment

Copy link
Copy Markdown
Member

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.

Lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bengl
bengl marked this pull request as draft May 12, 2022 06:24
@bengl

bengl commented May 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Looks like I submitted too soon again here.

While FunctionPrototypeBind does preserve length, it's preserving the length of runInAsyncScope, and not the fn in question. It will need to preserve the length the same way as the other code path, but I since I suspect that it will still be significantly faster, I'm going to leave this open in Draft mode until I can clean it up (hopefully in the next day or two).
mcollina reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bengl
async_hooks: faster AsyncResource#bind()
003b50b
@bengl
bengl force-pushed the bengl/faster-bind branch from 212ea52 to 003b50b Compare May 12, 2022 14:08
@bengl

bengl commented May 12, 2022

Copy link
Copy Markdown
Member Author

Setting this as "ready for review" again. It's now always doing an ObjectDefineProperty, but that's still about a 3.7x improvement on my machine.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bengl
bengl marked this pull request as ready for review May 12, 2022 14:15
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
mcollina
mcollina approved these changes May 12, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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.

LGTM

How did you found this was a bottleneck? :D

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bengl

bengl commented May 12, 2022

Copy link
Copy Markdown
Member Author

@mcollina .bind() is a pretty hot path with a lot of the new changes (and some upcoming ones) in our APM tracing library, so I was investigating it directly.
mcollina and tonivj5 reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request May 12, 2022
CI Reliability 2022-05-12 nodejs/reliability#275
Closed
32 tasks
@bengl

bengl commented May 12, 2022

Copy link
Copy Markdown
Member Author

Here's a benchmark run: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1136/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented May 12, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/43974/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Flarna
Flarna approved these changes May 12, 2022
View reviewed changes
lpinca
lpinca approved these changes May 12, 2022
View reviewed changes
@github-actions github-actions Bot mentioned this pull request May 13, 2022
CI Reliability 2022-05-13 nodejs/reliability#276
Open
33 tasks
JungMinu
JungMinu approved these changes May 13, 2022
View reviewed changes
puzpuzpuz
puzpuzpuz approved these changes May 13, 2022
View reviewed changes
legendecas
legendecas approved these changes May 13, 2022
View reviewed changes
RafaelGSS
RafaelGSS approved these changes May 13, 2022
View reviewed changes
Qard
Qard approved these changes May 13, 2022
View reviewed changes
This was referenced May 14, 2022
CI Reliability 2022-05-14 nodejs/reliability#277
Open
CI Reliability 2022-05-15 nodejs/reliability#278
Open
@nodejs-github-bot

nodejs-github-bot commented May 15, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44014/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen
RaisinTen approved these changes May 15, 2022
View reviewed changes
@github-actions github-actions Bot mentioned this pull request May 16, 2022
CI Reliability 2022-05-16 nodejs/reliability#279
Open
25 tasks
rochdev
rochdev reviewed May 16, 2022
View reviewed changes
Comment thread lib/async_hooks.js
value: this,
writable: true,
}
ObjectDefineProperty(bound, 'length', {

@rochdev rochdev May 16, 2022

Copy link
Copy Markdown
Contributor

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.

Why not just stop doing this completely? This is only one of the cases needed to preserve the original functionality as the function could also have other properties or symbols attached to it for example, and those wouldn't be copied. Maybe the best approach then would be to copy nothing and leave it to the user to decide what needs copying. Then the implementation will be as fast as it can be and will only become slower if the user decides that copying properties is needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
tniessen
tniessen approved these changes May 16, 2022
View reviewed changes
@github-actions github-actions Bot mentioned this pull request May 17, 2022
CI Reliability 2022-05-17 nodejs/reliability#280
Open
23 tasks
@nodejs-github-bot

nodejs-github-bot commented May 17, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44047/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request May 18, 2022
CI Reliability 2022-05-18 nodejs/reliability#281
Open
28 tasks
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@bengl bengl mentioned this pull request Nov 12, 2023
async_hooks: deprecate the AsyncResource.bind asyncResource property #46432
Merged
@bengl

bengl commented Nov 12, 2023

Copy link
Copy Markdown
Member Author

Closing since #46432 precludes this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bengl bengl closed this Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@Qard Qard Qard approved these changes
@lpinca lpinca lpinca approved these changes
@tniessen tniessen tniessen approved these changes
@JungMinu JungMinu JungMinu approved these changes
@legendecas legendecas legendecas approved these changes
@juanarbol juanarbol juanarbol approved these changes
@Flarna Flarna Flarna approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes
@RaisinTen RaisinTen RaisinTen approved these changes
+2 more reviewers
@rochdev rochdev rochdev left review comments
@puzpuzpuz puzpuzpuz puzpuzpuz approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

14 participants

@bengl @nodejs-github-bot @mcollina @Qard @lpinca @rochdev @tniessen @JungMinu @legendecas @juanarbol @Flarna @RafaelGSS @puzpuzpuz @RaisinTen
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.