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

http: correctly optimize debug function #37966

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Mar 29, 2021

Exporting a variable that will be mutated later doesn't work.

Refs: #37937

Exporting a variable that will be mutated later doesn't work.

Refs: nodejs#37937
@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Mar 29, 2021
@targos
Copy link
Member Author

targos commented Mar 29, 2021

@mcollina can you verify the fix on your benchmarking server?

@mcollina
Copy link
Member

Yes it does but not completely. There are probably a few others but this does improve things if landed on top of #37963 (nothing otherwise as the other commit is a dominant bottleneck).

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member


$ git diff
diff --git a/lib/internal/util/debuglog.js b/lib/internal/util/debuglog.js
index 3dc7a5157d..6eec7b28f2 100644
--- a/lib/internal/util/debuglog.js
+++ b/lib/internal/util/debuglog.js
@@ -81,7 +81,7 @@ function debuglog(set, cb) {
     debug = debuglogImpl(enabled, set);
     if (typeof cb === 'function')
       cb(debug);
-    debug(...new SafeArrayIterator(args));
+    debug(...args);
   };
   let enabled;
   let test = () => {
@@ -89,7 +89,7 @@ function debuglog(set, cb) {
     test = () => enabled;
     return enabled;
   };
-  const logger = (...args) => debug(...new SafeArrayIterator(args));
+  const logger = (...args) => debug(...args);
   ObjectDefineProperty(logger, 'enabled', {
     get() {
       return test();

Is still slightly faster than this fix alone (72k vs 70k).

Can we add this as well?

@targos
Copy link
Member Author

targos commented Mar 29, 2021


$ git diff
diff --git a/lib/internal/util/debuglog.js b/lib/internal/util/debuglog.js
index 3dc7a5157d..6eec7b28f2 100644
--- a/lib/internal/util/debuglog.js
+++ b/lib/internal/util/debuglog.js
@@ -81,7 +81,7 @@ function debuglog(set, cb) {
     debug = debuglogImpl(enabled, set);
     if (typeof cb === 'function')
       cb(debug);
-    debug(...new SafeArrayIterator(args));
+    debug(...args);
   };
   let enabled;
   let test = () => {
@@ -89,7 +89,7 @@ function debuglog(set, cb) {
     test = () => enabled;
     return enabled;
   };
-  const logger = (...args) => debug(...new SafeArrayIterator(args));
+  const logger = (...args) => debug(...args);
   ObjectDefineProperty(logger, 'enabled', {
     get() {
       return test();

Is still slightly faster than this fix alone (72k vs 70k).

Can we add this as well?

I can add it, but I honestly don't see how the difference can be measured, as this code path is hit only eight times during the entire process life, regardless of the number of HTTP requests.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

I can add it, but I honestly don't see how the difference can be measured, as this code path is hit only eight times during the entire process life, regardless of the number of HTTP requests.

I'll check it further and verify again.

@mcollina
Copy link
Member

This is ok to land, there are no other regression (I checked extensively)

@nodejs-github-bot
Copy link
Collaborator

@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 30, 2021
targos added a commit that referenced this pull request Mar 31, 2021
Exporting a variable that will be mutated later doesn't work.

Refs: #37937

PR-URL: #37966
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos
Copy link
Member Author

targos commented Mar 31, 2021

Landed in 3dee233

@targos targos closed this Mar 31, 2021
@targos targos deleted the fix-http-debug-regression branch March 31, 2021 16:22
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
Exporting a variable that will be mutated later doesn't work.

Refs: #37937

PR-URL: #37966
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants