FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: don't init Debug if it's not needed yet by bengl · Pull Request #8452 · nodejs/node · GitHub

/ node Public

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

Closed
bengl wants to merge 1 commit into
nodejs:masterfrom
bengl:deferdebuginitialize
ConversationCommits (1)ChecksFiles changed
Closed

util: don't init Debug if it's not needed yet#8452
bengl wants to merge 1 commit into
nodejs:masterfrom
bengl:deferdebuginitialize

Conversation

bengl commented Sep 8, 2016

Copy link
Copy Markdown
Member
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.

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 added the util Issues and PRs related to the built-in util module. label Sep 8, 2016

bengl commented Sep 8, 2016

Copy link
Copy Markdown
Member Author

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

addaleax commented Sep 8, 2016

Copy link
Copy Markdown
Member

LGTM pending CI

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

jasnell commented Sep 8, 2016

Copy link
Copy Markdown
Member

LGTM

mscdex reviewed Sep 8, 2016
View reviewed changes
Comment thread lib/util.js

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

mscdex Sep 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This comment should probably be moved also

bengl Sep 9, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

mscdex Sep 9, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Sep 8, 2016

Copy link
Copy Markdown
Member

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 commented Sep 9, 2016

Copy link
Copy Markdown
Member

CI failures now seem less problematic:

TL;DR: CI looks good:

bnoordhuis commented Sep 9, 2016

Copy link
Copy Markdown
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 commented Sep 9, 2016

Copy link
Copy Markdown
Contributor

LGTM

2 similar comments

thefourtheye commented Sep 10, 2016

Copy link
Copy Markdown
Contributor

LGTM

evanlucas commented Sep 13, 2016

Copy link
Copy Markdown
Contributor

LGTM

evanlucas commented Sep 13, 2016

Copy link
Copy Markdown
Contributor

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

bengl commented Sep 17, 2016

Copy link
Copy Markdown
Member Author

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
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 commented Sep 20, 2016

Copy link
Copy Markdown
Member

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

jasnell closed this Sep 20, 2016

bengl commented Sep 20, 2016

Copy link
Copy Markdown
Member Author

Don't mind at all 👍

MylesBorins commented Oct 6, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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
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 commented Dec 21, 2016

Copy link
Copy Markdown
Contributor

ping @nodejs/lts

ofrobots commented Dec 21, 2016

Copy link
Copy Markdown
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
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
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 mentioned this pull request Mar 9, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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.

Back | FazBrowse Home | New Git URL