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

util: don't init Debug if it's not needed yet #8452

Closed
wants to merge 1 commit into from
Closed

util: don't init Debug if it's not needed yet #8452

wants to merge 1 commit into from

Conversation

bengl
Copy link
Member

@bengl bengl commented Sep 8, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

util

Description of change

Because any call to util.inspect() with an object results in
inspectPromise() being called, Debug was being initialized even when
it's not needed. Instead, the initialization is placed after the
isPromise check.

@bengl
util: don't init Debug if it's not needed yet
4ab5ccb 
Because any call to util.inspect() with an object results in
inspectPromise() being called, Debug was being initialized even when
it's not needed. Instead, the initialization is placed after the
isPromise check.
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Sep 8, 2016
@bengl
Copy link
Member Author

bengl commented Sep 8, 2016

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

@addaleax
Copy link
Member

addaleax commented Sep 8, 2016

LGTM pending CI

(also, TIL Node has awesome things like vm.runInDebugContext('Debug')!)

@jasnell
Copy link
Member

jasnell commented Sep 8, 2016

LGTM

mscdex
mscdex reviewed Sep 8, 2016
View reviewed changes
lib/util.js
@@ -301,10 +301,10 @@ function ensureDebugIsInitialized() {


function inspectPromise(p) {
ensureDebugIsInitialized();
// Only create a mirror if the object is a Promise.
Copy link
Contributor

@mscdex mscdex Sep 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This comment should probably be moved also

Copy link
Member Author

Choose a reason for hiding this comment

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

Where to? Wouldn't the best place for the comment about checking whether it's a promise be right before the line that checks if it's a promise?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess to me it's a little confusing since the comment is talking about creating a mirror which isn't done on the next line and the function name does not indicate that a mirror is being created. shrug

@Trott
Copy link
Member

Trott commented Sep 8, 2016

CI failures are strange, especially SmartOS. I suspect temporary infra borkage. Let's try again:

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

@Trott
Copy link
Member

Trott commented Sep 9, 2016

CI failures now seem less problematic:

TL;DR: CI looks good:

@bnoordhuis
Copy link
Member

LGTM. Transient error on the arm buildbot and sequential/test-crypto-timing-safe-equal failure on one of the linux buildbots. Unlikely to be caused by this PR.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 9, 2016

LGTM

2 similar comments
@thefourtheye
Copy link
Contributor

LGTM

@evanlucas
Copy link
Contributor

LGTM

@evanlucas
Copy link
Contributor

trying CI again: https://ci.nodejs.org/job/node-test-pull-request/4024/

@bengl
Copy link
Member Author

bengl commented Sep 17, 2016

kicking CI one more time in hopes of all green: https://ci.nodejs.org/job/node-test-pull-request/4078/

jasnell pushed a commit that referenced this pull request Sep 20, 2016
@bengl @jasnell
util: don't init Debug if it's not needed yet
be07458 
Because any call to util.inspect() with an object results in
inspectPromise() being called, Debug was being initialized even when
it's not needed. Instead, the initialization is placed after the
isPromise check.

PR-URL: #8452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

The failures look like flaky issues to me. Hope you don't mind that I went ahead and landed in be07458

@jasnell jasnell closed this Sep 20, 2016
@bengl
Copy link
Member Author

bengl commented Sep 20, 2016

Don't mind at all 👍

@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 6, 2016 •
edited
Loading

@bengl @addaleax @jasnell should this be backported after some time on current?

edit: fwiw this lands cleanly

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@bengl @Fishrock123
util: don't init Debug if it's not needed yet
38be155 
Because any call to util.inspect() with an object results in
inspectPromise() being called, Debug was being initialized even when
it's not needed. Instead, the initialization is placed after the
isPromise check.

PR-URL: #8452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@MylesBorins
Copy link
Contributor

ping @nodejs/lts

@ofrobots
Copy link
Contributor

This is a safe performance fix. I would be mildly in favour of backporting.

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@bengl @MylesBorins
util: don't init Debug if it's not needed yet
281999e 
Because any call to util.inspect() with an object results in
inspectPromise() being called, Debug was being initialized even when
it's not needed. Instead, the initialization is placed after the
isPromise check.

PR-URL: #8452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@bengl @MylesBorins
util: don't init Debug if it's not needed yet
2c84601 
Because any call to util.inspect() with an object results in
inspectPromise() being called, Debug was being initialized even when
it's not needed. Instead, the initialization is placed after the
isPromise check.

PR-URL: #8452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.