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.

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

lib: improve debuglog() performance #32260

Merged
merged 1 commit into from
May 30, 2020
Merged

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex 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

@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Mar 14, 2020
@nodejs-github-bot nodejs-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
function debuglog(set, cb) {
let debug = (...args) => {
// Only invokes debuglogImpl() when the debug function is
// called for the first time.
debug = debuglogImpl(set);
if (typeof cb === 'function')
cb(debug);
debug(...args);
};
return (...args) => {
debug(...args);
};
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
function debuglog(set, cb) {
let debug = (...args) => {
// Only invokes debuglogImpl() when the debug function is
// called for the first time.
debug = debuglogImpl(set);
if (typeof cb === 'function')
cb(debug);
debug(...args);
};
return (...args) => {
debug(...args);
};
function debuglog(set) {
let debug = (...args) => {
// Only invokes debuglogImpl() when the debug function is
// called for the first time.
debug = debuglogImpl(set);
debug(...args);
};
const state = { 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) {
const state = { fired: false, wrapFn: undefined, target, type, listener };
const wrapped = onceWrapper.bind(state);
wrapped.listener = listener;
state.wrapFn = wrapped;
return wrapped;
}

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
Contributor Author

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.

@mscdex mscdex force-pushed the util-debuglog-perf branch from 083cb02 to de4fdd1 Compare May 30, 2020 18:23
@mscdex
Copy link
Contributor Author

mscdex commented May 30, 2020

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

@mscdex mscdex force-pushed the util-debuglog-perf branch from de4fdd1 to 58d7130 Compare May 30, 2020 21:21
PR-URL: nodejs#32260
Reviewed-By: James M Snell <jasnell@gmail.com>
@mscdex mscdex force-pushed the util-debuglog-perf branch from 58d7130 to c24b74a Compare May 30, 2020 21:24
@mscdex mscdex merged commit c24b74a into nodejs:master May 30, 2020
@mscdex mscdex deleted the util-debuglog-perf branch May 30, 2020 21:27
codebytere pushed a commit that referenced this pull request Jun 27, 2020
PR-URL: #32260
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
PR-URL: #32260
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 27, 2020
PR-URL: #32260
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 28, 2020
PR-URL: #32260
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants