Skip to content

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

Closed
RaisinTen wants to merge 0 commit intonodejs:masterfrom
RaisinTen:module/add-optional-chaining-in-cjs-loader
Closed

module: use optional chaining in cjs/loader.js#37238
RaisinTen wants to merge 0 commit intonodejs:masterfrom
RaisinTen:module/add-optional-chaining-in-cjs-loader

Conversation

@RaisinTen
Copy link
Copy Markdown
Member

@RaisinTen RaisinTen commented Feb 5, 2021

No description provided.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Feb 5, 2021

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

@RaisinTen
Copy link
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
Member Author

RaisinTen commented Feb 5, 2021

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

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Lxxyx Lxxyx Lxxyx approved these changes

@aduh95 aduh95 aduh95 approved these changes

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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