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.

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

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

Closed

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Jan 3, 2017

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 Fishrock123 added the util Issues and PRs related to the built-in util module. label Jan 3, 2017
@targos
Copy link
Member

targos commented Jan 3, 2017

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

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

@evanlucas
Copy link
Contributor

evanlucas commented Jan 3, 2017

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

bmeurer commented Jan 3, 2017

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

Thanks for the clarification @bmeurer

@targos
Copy link
Member

targos commented Jan 4, 2017

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

bmeurer commented Jan 4, 2017

Yes.

@Fishrock123
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants