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

util: avoid leaking `arguments` in `_deprecate()`. by Fishrock123 · Pull Request #10594 · nodejs/node · GitHub

/ node Public

util: avoid leaking arguments in _deprecate(). - #10594

Closed
Fishrock123 wants to merge 1 commit into
nodejs:masterfrom
Fishrock123:avoid-util-arguments-leak
ConversationCommits (1)ChecksFiles changed
Closed

util: avoid leaking arguments in _deprecate().#10594
Fishrock123 wants to merge 1 commit into
nodejs:masterfrom
Fishrock123:avoid-util-arguments-leak

Conversation

Fishrock123 commented Jan 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Refs: #10323

I didn't bother to benchmark it, but this probably deopts this function in all calls to a deprecated class / constructor.

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

util

Fishrock123 added the util Issues and PRs related to the built-in util module. label Jan 3, 2017

targos commented Jan 3, 2017

Copy link
Copy Markdown
Member

I'm not sure there is a deopt in this case.

I ran this simple test:

var util = require('util');
function MyClass(a, b, c) {
  this.a = a;
  this.b = b;
  this.c = c;
}
var Deprecated = util.deprecate(MyClass);
while(true) {
  new Deprecated(1, 2, 3);
}
Result 
[marking 0x1138891d7141 <JS Function deprecated (SharedFunctionInfo 0x327d3dd127a1)> for optimized recompilation, reason: small function, ICs with typeinfo: 7/10 (70%), generic ICs: 0/10 (0%)]
[compiling method 0x1138891d7141 <JS Function deprecated (SharedFunctionInfo 0x327d3dd127a1)> using TurboFan]
[optimizing 0x1138891d7141 <JS Function deprecated (SharedFunctionInfo 0x327d3dd127a1)> - took 0.288, 0.309, 0.076 ms]
[completed optimizing 0x1138891d7141 <JS Function deprecated (SharedFunctionInfo 0x327d3dd127a1)>]
[marking 0x327d3dd8d1b9 <JS Function (SharedFunctionInfo 0x327d3dd65b69)> for optimized recompilation, reason: small function, ICs with typeinfo: 2/3 (66%), generic ICs: 0/3 (0%)]
[compiling method 0x327d3dd8d1b9 <JS Function (SharedFunctionInfo 0x327d3dd65b69)> using Crankshaft OSR]
[optimizing 0x327d3dd8d1b9 <JS Function (SharedFunctionInfo 0x327d3dd65b69)> - took 0.318, 0.210, 0.084 ms]
[marking 0x327d3dd8d0a1 <JS Function MyClass (SharedFunctionInfo 0x327d3dd65c61)> for optimized recompilation, reason: not much type info but very hot, ICs with typeinfo: 0/3 (0%), generic ICs: 3/3 (100%)]
[compiling method 0x327d3dd8d0a1 <JS Function MyClass (SharedFunctionInfo 0x327d3dd65c61)> using Crankshaft]
[optimizing 0x327d3dd8d0a1 <JS Function MyClass (SharedFunctionInfo 0x327d3dd65c61)> - took 0.040, 0.072, 0.010 ms]
[completed optimizing 0x327d3dd8d0a1 <JS Function MyClass (SharedFunctionInfo 0x327d3dd65c61)>]

/cc @bmeurer

Fishrock123 commented Jan 3, 2017

Copy link
Copy Markdown
Contributor Author

Interesting. Perhaps Reflect.construct() is another exception like apply()?

evanlucas commented Jan 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

iirc Reflect.apply and Reflect.construct are optimized similar to how Function#apply is. (I'll try and find where I read that)

Edit: https://docs.google.com/document/d/1DvDx3Xursn1ViV5k4rT4KB8HBfBb2GdUy3wzNfJWcKM/edit#heading=h.cd88kfmuxw1k is where I read that

bmeurer commented Jan 3, 2017

Copy link
Copy Markdown
Member

No, Crankshaft doesn't know about Reflect.construct or Reflect.apply, and TurboFan doesn't have the arguments issue. Long-term TurboFan will optimize it similarly to Function.prototype.apply, as described in the document, but that's just the plan for now.

evanlucas commented Jan 4, 2017

Copy link
Copy Markdown
Contributor

Thanks for the clarification @bmeurer

targos commented Jan 4, 2017

Copy link
Copy Markdown
Member

So here, we don't see a deopt because the function is compiled with TurboFan and it is compiled with TurboFan because it's using new.target. Correct?

bmeurer commented Jan 4, 2017

Copy link
Copy Markdown
Member

Yes.

Fishrock123 commented Jan 4, 2017

Copy link
Copy Markdown
Contributor Author

Interesting, sounds like this is good to close? Lmk if it should be reopened for whatever reason.

Fishrock123 closed this Jan 4, 2017
vsemozhetbyt mentioned this pull request Feb 1, 2017
Closed
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.

4 participants

Back | FazBrowse Home | New Git URL