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

Speed up promise introspection #3130

Merged
merged 2 commits into from
Sep 30, 2015
Merged

Speed up promise introspection #3130

merged 2 commits into from
Sep 30, 2015

Conversation

bnoordhuis
Copy link
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/

@bnoordhuis bnoordhuis mentioned this pull request Sep 30, 2015
Closed
@thefourtheye
Copy link
Contributor

LGTM

targos
targos reviewed Sep 30, 2015
View reviewed changes
lib/util.js
const result = runInDebugContext('[Debug, ObjectIsPromise]');
Debug = result[0];
ObjectIsPromise = result[1];
}
Copy link
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
Copy link
Contributor

LGTM

@mscdex mscdex 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 bnoordhuis force-pushed the speed-up-promise-introspection branch from 076dd53 to dbe4844 Compare September 30, 2015 21:44
@bnoordhuis bnoordhuis closed this Sep 30, 2015
@bnoordhuis bnoordhuis deleted the speed-up-promise-introspection branch September 30, 2015 21:44
@bnoordhuis bnoordhuis merged commit dbe4844 into nodejs:master Sep 30, 2015
@bnoordhuis
Copy link
Member Author

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
Copy link
Contributor

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

@bnoordhuis
Copy link
Member Author

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
Copy link
Contributor

Sorry I meant the util changes. Damn autocorrect :D

@bnoordhuis
Copy link
Member Author

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

@thefourtheye
Copy link
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>
@rvagg rvagg mentioned this pull request Oct 3, 2015
Closed
@rvagg
Copy link
Member

rvagg commented Oct 5, 2015

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

@targos
Copy link
Member

targos commented Oct 5, 2015

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

@rvagg
Copy link
Member

rvagg commented Oct 5, 2015

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
@ChALkeR ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 16, 2016
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
performance Issues and PRs related to the performance of Node.js. 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.
7 participants
@bnoordhuis @thefourtheye @evanlucas @rvagg @targos @mscdex @ChALkeR