Skip to content

lib: refactor to use missing primordials - #38785

Closed
VoltrexKeyva wants to merge 1 commit into
nodejs:masterfrom
VoltrexKeyva:patch-3
Closed

VoltrexKeyva wants to merge 1 commit into
nodejs:masterfrom
VoltrexKeyva:patch-3

Conversation

@VoltrexKeyva

Copy link
Copy Markdown
Contributor

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

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 left a comment

Copy link
Copy Markdown
Contributor

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

aduh95 commented May 23, 2021

Copy link
Copy Markdown
Contributor

@mscdex

mscdex commented May 24, 2021

Copy link
Copy Markdown
Contributor

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

aduh95 commented May 24, 2021

Copy link
Copy Markdown
Contributor

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

@aduh95 aduh95 added the needs-benchmark-ci PRs that need a benchmark CI run. label May 27, 2021

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor 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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Sponsor
SponsoredKunjungi sekarang
Promo