Skip to content

Conversation

@mscdex
Copy link
Contributor

@mscdexmscdex commented Mar 14, 2020

Example benchmark results:

 confidence improvement accuracy (*) (**) (***) streams/pipe-object-mode.js n=5000000 *** 27.10 % ±1.46% ±1.95% ±2.55% streams/pipe.js n=5000000 *** 23.04 % ±1.54% ±2.08% ±2.76% streams/readable-boundaryread.js type='buffer' n=2000 ** 3.22 % ±2.19% ±2.92% ±3.83% streams/readable-readall.js n=5000 *** 9.04 % ±2.51% ±3.35% ±4.39% streams/readable-unevenread.js n=1000 *** 6.38 % ±0.63% ±0.84% ±1.10% 

Improvements come from optimizing the existing function returned by debuglog() but also by adding an optional callback that can be used to get a reference to the actual underlying debug output function to avoid calling into the unnecessary wrapper function.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@mscdexmscdex added the performance Issues and PRs related to the performance of Node.js. label Mar 14, 2020
@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 14, 2020
@nodejs-github-bot
Copy link
Collaborator

Comment on lines +63 to 74
Copy link
Member

Choose a reason for hiding this comment

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

What about this?

Suggested change
functiondebuglog(set,cb){
letdebug=(...args)=>{
// Only invokes debuglogImpl() when the debug function is
// called for the first time.
debug=debuglogImpl(set);
if(typeofcb==='function')
cb(debug);
debug(...args);
};
return(...args)=>{
debug(...args);
};
functiondebuglog(set){
letdebug=(...args)=>{
// Only invokes debuglogImpl() when the debug function is
// called for the first time.
debug=debuglogImpl(set);
debug(...args);
};
conststate={called: false};
return((...args)=>{
if(!this.called){
debug(...args);
this.called=true;
}else{
debug(...args);
}
}).bind(state);

just like

node/lib/events.js

Lines 426 to 432 in ec204d8

function_onceWrap(target,type,listener){
conststate={fired: false,wrapFn: undefined, target, type, listener };
constwrapped=onceWrapper.bind(state);
wrapped.listener=listener;
state.wrapFn=wrapped;
returnwrapped;
}

Copy link
Member

Choose a reason for hiding this comment

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

because I don't think let debug = fn('xxx', v => debug = v) is a good code style

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The suggested change is basically what the code was doing before. Having a callback is the only way to be able to avoid the wrapper function and utilizing it provides a further, noticeable performance improvement.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I understand

Copy link
Member

Choose a reason for hiding this comment

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

Fairly certain this is because of the way V8 optimizes but that's only a guess. It's interesting that it's such a big perf difference.

@mscdexmscdex mentioned this pull request Mar 16, 2020
4 tasks
@mscdexmscdexforce-pushed the util-debuglog-perf branch from 083cb02 to de4fdd1CompareMay 30, 2020 18:23
@mscdex
Copy link
ContributorAuthor

Results again with current master:

 confidence improvement accuracy (*) (**) (***) streams/pipe-object-mode.js n=5000000 *** 40.88 % ±2.68% ±3.57% ±4.65% streams/pipe.js n=5000000 *** 31.51 % ±1.83% ±2.44% ±3.18% streams/readable-boundaryread.js type='buffer' n=2000 *** 4.53 % ±0.41% ±0.55% ±0.72% streams/readable-readall.js n=5000 *** 10.17 % ±2.23% ±2.97% ±3.88% streams/readable-unevenread.js n=1000 *** 4.25 % ±0.82% ±1.09% ±1.43% 

@nodejs-github-bot
Copy link
Collaborator

@mscdexmscdexforce-pushed the util-debuglog-perf branch from de4fdd1 to 58d7130CompareMay 30, 2020 21:21
@mscdexmscdexforce-pushed the util-debuglog-perf branch from 58d7130 to c24b74aCompareMay 30, 2020 21:24
@mscdexmscdex merged commit c24b74a into nodejs:masterMay 30, 2020
@mscdexmscdex deleted the util-debuglog-perf branch May 30, 2020 21:27
This was referenced Jun 12, 2020
codebytere pushed a commit that referenced this pull request Jun 27, 2020
@codebyterecodebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
addaleax pushed a commit that referenced this pull request Sep 27, 2020
addaleax pushed a commit that referenced this pull request Sep 28, 2020
@codebyterecodebytere mentioned this pull request Sep 28, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / srcIssues and PRs related to general changes in the lib or src directory.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@mscdex@nodejs-github-bot@jasnell@himself65