Skip to content

perf: avoid static AsyncResource.bind - #173

Merged
bjohansebas merged 2 commits into
masterfrom
fix-perf-async-context
Jul 9, 2026
Merged

perf: avoid static AsyncResource.bind#173
bjohansebas merged 2 commits into
masterfrom
fix-perf-async-context

Conversation

@bjohansebas

@bjohansebas bjohansebas commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

The purpose of wrapping the callback in an AsyncResource is to propagate the async execution context, so that AsyncLocalStorage (per-request data) and APM tools built on async_hooks (Datadog, New Relic, OpenTelemetry) see the correct context inside the callback. That behavior remains exactly the same: both implementations invoke done within the context captured when getRawBody was called. I verified this above (ctx.id = req-1 in both cases) and across all 119 tests.

They also both create exactly one AsyncResource per request, just as before, so the number of init/destroy events is unchanged. Any tooling that counts async resources will observe no difference.

The only observable difference is .asyncResource.

The static helper attaches an .asyncResource property to the bound function. this implementation does not. And that's actually a good thing: that property is deprecated. Node already emits DeprecationWarning: The asyncResource property on bound functions is deprecated (DEP0172).

@bjohansebas
bjohansebas merged commit d765d5c into master Jul 9, 2026
27 checks passed
@bjohansebas
bjohansebas deleted the fix-perf-async-context branch July 9, 2026 19:01
@bjohansebas

Copy link
Copy Markdown
Collaborator Author

cc: @Phillip9587

@bjohansebas

bjohansebas commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

with body-parser
before:

size mode | 3.0.2 req/s | master req/s | 302 p99 mstr p99 | delta winner

small raw | 28277.03 | 22710.16 | 3.59ms 9.93ms | -19.7% 302
small text | 26362.05 | 21528.37 | 4.11ms 9.37ms | -18.3% 302
medium raw | 9729.47 | 8763.96 | 25.13ms 32.39ms | -9.9% 302
medium text | 10388.80 | 7236.24 | 17.46ms 78.57ms | -30.3% 302
large raw | 306.53 | 295.49 | 313.04ms 296.96ms | -3.6% 302
large text | 234.58 | 235.93 | 291.52ms 396.63ms | +0.6% master

after:

scenario | 3.0.2 | master | delta

small/raw | 28375 | 28491 | +0.4%
small/text | 25550 | 26591 | +4.1%
medium/raw | 9451 | 9406 | -0.5%
medium/text | 10334 | 11187 | +8.3%
large/raw | 302 | 308 | +2.0%
large/text | 226 | 238 | +5.3%

@Phillip9587

Copy link
Copy Markdown
Collaborator

First of all, I really appreciate all the work you've been putting into the project. Your contributions have been a huge benefit to the ecosystem, and it's great to see how much effort and energy you're investing.

That said, I'd like to ask one thing: please leave your PRs open a bit longer for reviewers like me. Your last few PRs were all merged within about an hour, which didn't leave any time for others to review them.

I know you're eager to ship improvements to the whole ecosystem, and I appreciate that. But please leave maintainers and reviewers some time to evaluate the changes, discuss alternatives, and consider the broader implications before merging.

In my opinion, that's an important part of a healthy review process and leads to better long-term decisions.

@bjohansebas

Copy link
Copy Markdown
Collaborator Author

But there isn't really anyone else here, and this project isn't part of Express.js, which is why I've been moving changes quickly. I'm also making sure everything is thoroughly verified before merging.

If you'd like to get involved, you're more than welcome. I mainly wanted to let you know about the change because I believe something similar was done in on-finished.

@Phillip9587

Copy link
Copy Markdown
Collaborator

Thanks for the clarification. And just to be clear, I really appreciate the work you're doing here. My only request is to leave PRs open a bit longer before merging. I'd love to have some time to review changes and provide feedback. Even if it feels a bit slower, giving reviewers (who also have work and other commitments) a chance to take a look would be greatly appreciated.

@Phillip9587

Copy link
Copy Markdown
Collaborator

I'd also be more than happy to review and help with changes to raw-body. I'd be honored to contribute and help out where I can.

@bjohansebas

Copy link
Copy Markdown
Collaborator Author

Cool. Well, I'm planning to release this version on Saturday, so if you want to take a look, feel free. If you need more time, just let me know, and we can make any final tweaks before the release.

@bjohansebas

Copy link
Copy Markdown
Collaborator Author

@Phillip9587 , I'm going to add you as a committer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants