Skip to content

Speed up promise introspection - #3130

Merged
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:speed-up-promise-introspection
Sep 30, 2015
Merged

Speed up promise introspection#3130
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:speed-up-promise-introspection

Conversation

@bnoordhuis

@bnoordhuisbnoordhuis commented Sep 30, 2015

Copy link
Copy Markdown
Member

R=@vkurchatkin | @thefourtheye | @targos?
#3107 for background. I realized we don't have to wait for a V8 upgrade.

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

@bnoordhuisbnoordhuis mentioned this pull request Sep 30, 2015
Closed
@thefourtheye

thefourtheye commented Sep 30, 2015

Copy link
Copy Markdown
Contributor

LGTM

targos
targos reviewed Sep 30, 2015
View reviewed changes
Comment threadlib/util.js

@targostargosSep 30, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To prepare the ground for the use of MakeMirror in #3119, may I suggest to move this to a separate function ? setupDebugObjects() for example.

@evanlucas

evanlucas commented Sep 30, 2015

Copy link
Copy Markdown
Contributor

LGTM

@mscdexmscdex added the util Issues and PRs related to the built-in util module. label Sep 30, 2015
@bnoordhuis
util: optimize promise introspection
9a1bc4e
Use V8's builtin ObjectIsPromise() to check that the value is a promise
before creating the promise mirror. Reduces garbage collector strain
in the (common) non-promise case, which is beneficial when inspecting
deep object graphs.
PR-URL: nodejs#3130
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@bnoordhuis
deps: backport f782159 from v8's upstream
dbe4844
Backport f78215962bf5de9d47c022e7baa3952d0bf6d17f from V8's upstream
to speed up promise introspection.
Original commit message:
Remove obsolete try/catch from ObjectIsPromise().
Review URL: https://codereview.chromium.org/1367123003
Cr-Commit-Position: refs/heads/master@{nodejs#30966}
PR-URL: nodejs#3130
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@bnoordhuis
bnoordhuisforce-pushed the speed-up-promise-introspection branch from 076dd53 to dbe4844CompareSeptember 30, 2015 21:44
@bnoordhuis
bnoordhuis deleted the speed-up-promise-introspection branch September 30, 2015 21:44
@bnoordhuis
bnoordhuis merged commit dbe4844 into nodejs:masterSep 30, 2015
@bnoordhuis

bnoordhuis commented Sep 30, 2015

Copy link
Copy Markdown
MemberAuthor

Thanks everyone, landed in dbe4844.

@targos I broke out the initialization into a separate function. It's kind of against my principles to make changes after a LGTM but well, just this time, because you asked.

@thefourtheye

thefourtheye commented Oct 1, 2015

Copy link
Copy Markdown
Contributor

Ben, can you please link the until commit as well?

@bnoordhuis

bnoordhuis commented Oct 1, 2015

Copy link
Copy Markdown
MemberAuthor

The what now? You mean the original commit? That's bnoordhuis/node@bdfee10 but that's dead now.

EDIT: bnoordhuis/io.js@bdfee10 works though.

@thefourtheye

thefourtheye commented Oct 1, 2015

Copy link
Copy Markdown
Contributor

Sorry I meant the util changes. Damn autocorrect :D

@bnoordhuis

bnoordhuis commented Oct 1, 2015

Copy link
Copy Markdown
MemberAuthor

Ah, sure. That's 3f62c40...dbe4844.

@thefourtheye

thefourtheye commented Oct 1, 2015

Copy link
Copy Markdown
Contributor

Thanks :-)

bnoordhuis added a commit that referenced this pull request Oct 2, 2015
@bnoordhuis@rvagg
util: optimize promise introspection
d8ab4e1
Use V8's builtin ObjectIsPromise() to check that the value is a promise
before creating the promise mirror. Reduces garbage collector strain
in the (common) non-promise case, which is beneficial when inspecting
deep object graphs.
PR-URL: #3130
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
bnoordhuis added a commit that referenced this pull request Oct 2, 2015
@bnoordhuis@rvagg
deps: backport f782159 from v8's upstream
925b29f
Backport f78215962bf5de9d47c022e7baa3952d0bf6d17f from V8's upstream
to speed up promise introspection.
Original commit message:
Remove obsolete try/catch from ObjectIsPromise().
Review URL: https://codereview.chromium.org/1367123003
Cr-Commit-Position: refs/heads/master@{#30966}
PR-URL: #3130
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@rvaggrvagg mentioned this pull request Oct 3, 2015
Closed
@rvagg

rvagg commented Oct 5, 2015

Copy link
Copy Markdown
Member

is this being upstreamed? do we have an external link or something?

@targos

targos commented Oct 5, 2015

Copy link
Copy Markdown
Member

@rvagg you mean the V8 change ? It's already upstream. The link is in the commit message of dbe4844

@rvagg

rvagg commented Oct 5, 2015

Copy link
Copy Markdown
Member

derp, I see it in the commit msgs now f782159

rvagg added a commit that referenced this pull request Oct 5, 2015
@rvagg
2015-10-05, Version 4.1.2 (Stable) Release
85df6ad
Notable changes
* http:
- Fix out-of-order 'finish' event bug in pipelining that can abort
execution, fixes DoS vulnerability CVE-2015-7384
(Fedor Indutny) #3128
- Account for pending response data instead of just the data on the
current request to decide whether pause the socket or not
(Fedor Indutny) #3128
* libuv: Upgraded from v1.7.4 to v1.7.5, see release notes for details
(Saúl Ibarra Corretgé) #3010
- A better rwlock implementation for all Windows versions
- Improved AIX support
* v8:
- Upgraded from v4.5.103.33 to v4.5.103.35 (Ali Ijaz Sheikh) #3117
- Backported f782159 from v8's upstream to help speed up Promise
introspection (Ben Noordhuis) #3130
- Backported c281c15 from v8's upstream to add JSTypedArray length
in post-mortem metadata (Julien Gilli) #3031
PR-URL: #3128
rvagg added a commit that referenced this pull request Oct 5, 2015
@rvagg
2015-10-05, Version 4.1.2 (Stable) Release
820735d
Notable changes
* http:
- Fix out-of-order 'finish' event bug in pipelining that can abort
execution, fixes DoS vulnerability CVE-2015-7384
(Fedor Indutny) #3128
- Account for pending response data instead of just the data on the
current request to decide whether pause the socket or not
(Fedor Indutny) #3128
* libuv: Upgraded from v1.7.4 to v1.7.5, see release notes for details
(Saúl Ibarra Corretgé) #3010
- A better rwlock implementation for all Windows versions
- Improved AIX support
* v8:
- Upgraded from v4.5.103.33 to v4.5.103.35 (Ali Ijaz Sheikh) #3117
- Backported f782159 from v8's upstream to help speed up Promise
introspection (Ben Noordhuis) #3130
- Backported c281c15 from v8's upstream to add JSTypedArray length
in post-mortem metadata (Julien Gilli) #3031
PR-URL: #3128
@ChALkeRChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 16, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

performanceIssues and PRs related to the performance of Node.js.utilIssues 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.

7 participants

@bnoordhuis@thefourtheye@evanlucas@rvagg@targos@mscdex@ChALkeR