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.

Sign up for GitHub

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

Jump to bottom

lib: refactor to use missing primordials #38785

Closed
wants to merge 1 commit into from
Closed

lib: refactor to use missing primordials #38785

wants to merge 1 commit into from

Conversation

VoltrexKeyva
Copy link
Member

The async_hooks internal module is not using some of the primordials, we should use primordials whenever possible for consistency.

@VoltrexKeyva
lib: refactor to use missing primordials
dfef567 
The `async_hooks` internal module is not using some of the primordials, we should use primordials whenever possible for consistency.
@github-actions github-actions bot added async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run. labels May 23, 2021
aduh95
aduh95 requested changes May 23, 2021
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Those were reverted in https://github.com/nodejs/node/pull/38248/files#diff-708abd8609bdfbfcb8abf61f945a9fb86b0e35f5a891f607ae1118ce8677f0e1 due to performance issue. This should not land until benchmarks are run to confirm it does not impact performance.

@aduh95
Copy link
Contributor

aduh95 commented May 23, 2021

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1027/

@mscdex
Copy link
Contributor

mscdex commented May 24, 2021

Is there something wrong with the benchmark machine? For some reason it keeps stalling/hanging or maybe the connection to Jenkins is silently being severed? I've tried restarting the job a couple times now. Normally the async_hooks benchmarks should only take about 50 minutes or less on a decent modern machine....

@aduh95
Copy link
Contributor

aduh95 commented May 24, 2021

I think there's something wrong in the configuration that makes benchmark using an http server to hang.

@aduh95 aduh95 mentioned this pull request May 24, 2021
Closed
@aduh95 aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label May 27, 2021
mcollina
mcollina requested changes May 27, 2021
View reviewed changes
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.

Don’t land this, this is an incredibly hot path.

@VoltrexKeyva
Copy link
Member Author

Seems like this is actually a really hot path, might be dangerous to land this; closing.

@VoltrexKeyva VoltrexKeyva deleted the patch-3 branch May 28, 2021 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina requested changes

@aduh95 aduh95 aduh95 requested changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. needs-benchmark-ci PR that need a benchmark CI run. 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.
6 participants
@VoltrexKeyva @aduh95 @mscdex @mcollina @jasnell @addaleax