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

module: use optional chaining in cjs/loader.js #37238

Closed
wants to merge 0 commits into from
Closed

module: use optional chaining in cjs/loader.js #37238

wants to merge 0 commits into from

Conversation

RaisinTen
Copy link
Contributor

No description provided.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35961/

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Feb 5, 2021 •
edited
Loading

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

No significant perf regressions 
20:33:02 module/module-loader-deep.jscache='false' files=1000 ext='.js'                            *     -3.51 %       ±3.25%  ±4.32%  ±5.63%
20:33:02 module/module-loader.jscache='false' n=1 files=500 dir='rel' name='/index.js'             *     -3.80 %       ±2.97%  ±3.95%  ±5.14%
20:33:02 module/module-loader.jscache='true' n=1000 files=500 dir='abs' name='/'                   *     -1.81 %       ±1.39%  ±1.86%  ±2.44%

aduh95
aduh95 approved these changes Feb 5, 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.

LGTM if benchmark results don't show perf regression

@RaisinTen
Copy link
Contributor Author

@aduh95 benchmark looks good! 🎉

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 5, 2021
@Trott
Copy link
Member

Trott commented Feb 6, 2021

Non-blocking from me, but I think whether the benchmarks look good in this case may be a matter of opinion. It shows three statistically-significant (but also, yes, small) regressions. Might be interesting to run again to see if they are persistent or not.

Benchmark CI re-run for comparison: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/937/

@Trott
Copy link
Member

Trott commented Feb 9, 2021

Benchmark re-run confirms no significant changes in benchmark results.

Trott pushed a commit to RaisinTen/node that referenced this pull request Feb 9, 2021
@RaisinTen @Trott
module: use optional chaining in cjs/loader.js
fdd7a87 
PR-URL: nodejs#37238
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott Trott closed this Feb 9, 2021
@Trott Trott force-pushed the module/add-optional-chaining-in-cjs-loader branch from 5abd9c2 to fdd7a87 Compare February 9, 2021 00:40
@Trott
Copy link
Member

Trott commented Feb 9, 2021

Landed in fdd7a87

@RaisinTen RaisinTen deleted the module/add-optional-chaining-in-cjs-loader branch February 9, 2021 09:28
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@RaisinTen @danielleadams
module: use optional chaining in cjs/loader.js
a693baa 
PR-URL: #37238
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This was referenced Feb 16, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@Lxxyx Lxxyx Lxxyx approved these changes

@ExE-Boss ExE-Boss ExE-Boss approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@RaisinTen @nodejs-github-bot @Trott @benjamingr @ExE-Boss @Lxxyx @aduh95 @targos