Move from instanceof to new.target check in all function-based classes #42042

mcollina · 2022-02-18T08:51:58Z

What is the problem this feature will solve?

I've recently discovered that there is a faster alternative to the instanceof check:

const max = 100000000

function InstanceOfClass () {
  if (!(this instanceof InstanceOfClass)) {
    return new InstanceOfClass()
  }
}

function NewTargetClass () {
  if (!new.target) {
    return new NewTargetClass()
  }
}

console.time('InstanceOfClass')
for (let i = 0; i < max; i++) {
  InstanceOfClass()
}
console.timeEnd('InstanceOfClass')

console.time('NewTargetClass')
for (let i = 0; i < max; i++) {
  NewTargetClass()
}
console.timeEnd('NewTargetClass')

On Node v16.14, this results in:

InstanceOfClass: 3.741s
NewTargetClass: 1.154s

The compound improvement from a massive migration might be worthwhile. Moreover this could be a "good first issue" task that we can fan-out.

cc @benjamingr @nodejs/tsc for awareness

What is the feature you are proposing to solve the problem?

As above

What alternatives have you considered?

No response

Edit: credit to xtx1130 who pointed this out to me at fastify/fastify-error#60.

The text was updated successfully, but these errors were encountered:

benjamingr · 2022-02-18T08:57:46Z

I agree let's add good-first-issue and start accepting PRs (ideally not for all of them to have many target PRs for new contributions), I see ±20 uses in our code:

lib/_http_agent.js:
  93  function Agent(options) {
  94:   if (!(this instanceof Agent))
  95      return new Agent(options);

lib/_http_server.js:
  368  function Server(options, requestListener) {
  369:   if (!(this instanceof Server)) return new Server(options, requestListener);
  370  

lib/_tls_common.js:
  71  function SecureContext(secureProtocol, secureOptions, minVersion, maxVersion) {
  72:   if (!(this instanceof SecureContext)) {
  73      return new SecureContext(secureProtocol, secureOptions, minVersion,

lib/_tls_wrap.js:
  1178  function Server(options, listener) {
  1179:   if (!(this instanceof Server))
  1180      return new Server(options, listener);

lib/domain.js:
  455    });
  456:   if (exports.active && !(this instanceof exports.Domain)) {
  457      this.domain = exports.active;

lib/https.js:
   52  function Server(opts, requestListener) {
   53:   if (!(this instanceof Server)) return new Server(opts, requestListener);
   54  

  176  function Agent(options) {
  177:   if (!(this instanceof Agent))
  178      return new Agent(options);

lib/net.js:
   283  function Socket(options) {
   284:   if (!(this instanceof Socket)) return new Socket(options);
   285    if (options?.objectMode) {

  1188  function Server(options, connectionListener) {
  1189:   if (!(this instanceof Server))
  1190      return new Server(options, connectionListener);

lib/readline.js:
  95  function Interface(input, output, completer, terminal) {
  96:   if (!(this instanceof Interface)) {
  97      return new Interface(input, output, completer, terminal);

lib/repl.js:
  228                      replMode) {
  229:   if (!(this instanceof REPLServer)) {
  230      return new REPLServer(prompt,

lib/tty.js:
  47  function ReadStream(fd, options) {
  48:   if (!(this instanceof ReadStream))
  49      return new ReadStream(fd, options);

  84  function WriteStream(fd) {
  85:   if (!(this instanceof WriteStream))
  86      return new WriteStream(fd);

lib/zlib.js:
730 function Deflate(opts) {
731: if (!(this instanceof Deflate))
732 return new Deflate(opts);

738 function Inflate(opts) {
739: if (!(this instanceof Inflate))
740 return new Inflate(opts);

746 function Gzip(opts) {
747: if (!(this instanceof Gzip))
748 return new Gzip(opts);

754 function Gunzip(opts) {
755: if (!(this instanceof Gunzip))
756 return new Gunzip(opts);

763 if (opts && opts.windowBits === 8) opts.windowBits = 9;
764: if (!(this instanceof DeflateRaw))
765 return new DeflateRaw(opts);

771 function InflateRaw(opts) {
772: if (!(this instanceof InflateRaw))
773 return new InflateRaw(opts);

779 function Unzip(opts) {
780: if (!(this instanceof Unzip))
781 return new Unzip(opts);

855 function BrotliCompress(opts) {
856: if (!(this instanceof BrotliCompress))
857 return new BrotliCompress(opts);

863 function BrotliDecompress(opts) {
864: if (!(this instanceof BrotliDecompress))
865 return new BrotliDecompress(opts);

lib/internal/cluster/worker.js:
  13  function Worker(options) {
  14:   if (!(this instanceof Worker))
  15      return new Worker(options);

lib/internal/crypto/certificate.js:
  40  function Certificate() {
  41:   if (!(this instanceof Certificate))
  42      return new Certificate();

lib/internal/crypto/cipher.js:
  143  function Cipher(cipher, password, options) {
  144:   if (!(this instanceof Cipher))
  145      return new Cipher(cipher, password, options);

  239  function Cipheriv(cipher, key, iv, options) {
  240:   if (!(this instanceof Cipheriv))
  241      return new Cipheriv(cipher, key, iv, options);

  269  function Decipher(cipher, password, options) {
  270:   if (!(this instanceof Decipher))
  271      return new Decipher(cipher, password, options);

  285  function Decipheriv(cipher, key, iv, options) {
  286:   if (!(this instanceof Decipheriv))
  287      return new Decipheriv(cipher, key, iv, options);

lib/internal/crypto/diffiehellman.js:
   88  function DiffieHellman(sizeOrKey, keyEncoding, generator, genEncoding) {
   89:   if (!(this instanceof DiffieHellman))
   90      return new DiffieHellman(sizeOrKey, keyEncoding, generator, genEncoding);

  148  function DiffieHellmanGroup(name) {
  149:   if (!(this instanceof DiffieHellmanGroup))
  150      return new DiffieHellmanGroup(name);

  247  function ECDH(curve) {
  248:   if (!(this instanceof ECDH))
  249      return new ECDH(curve);

lib/internal/crypto/hash.js:
   58  function Hash(algorithm, options) {
   59:   if (!(this instanceof Hash))
   60      return new Hash(algorithm, options);

  127  function Hmac(hmac, key, options) {
  128:   if (!(this instanceof Hmac))
  129      return new Hmac(hmac, key, options);

lib/internal/crypto/sig.js:
   54  function Sign(algorithm, options) {
   55:   if (!(this instanceof Sign))
   56      return new Sign(algorithm, options);

  195  function Verify(algorithm, options) {
  196:   if (!(this instanceof Verify))
  197      return new Verify(algorithm, options);

lib/internal/fs/streams.js:
  145  function ReadStream(path, options) {
  146:   if (!(this instanceof ReadStream))
  147      return new ReadStream(path, options);

  302  function WriteStream(path, options) {
  303:   if (!(this instanceof WriteStream))
  304      return new WriteStream(path, options);

lib/internal/streams/duplex.js:
  54  function Duplex(options) {
  55:   if (!(this instanceof Duplex))
  56      return new Duplex(options);

lib/internal/streams/passthrough.js:
  38  function PassThrough(options) {
  39:   if (!(this instanceof PassThrough))
  40      return new PassThrough(options);

lib/internal/streams/readable.js:
  186  function Readable(options) {
  187:   if (!(this instanceof Readable))
  188      return new Readable(options);

lib/internal/streams/transform.js:
  81  function Transform(options) {
  82:   if (!(this instanceof Transform))
  83      return new Transform(options);

That can/should be fixed and a few more that should probably be updated in the docs:

doc/api/stream.md:
  3107  function MyWritable(options) {
  3108:   if (!(this instanceof MyWritable))
  3109      return new MyWritable(options);

  3467  function MyReadable(options) {
  3468:   if (!(this instanceof MyReadable))
  3469      return new MyReadable(options);

  3805  function MyDuplex(options) {
  3806:   if (!(this instanceof MyDuplex))
  3807      return new MyDuplex(options);

  4002  function MyTransform(options) {
  4003:   if (!(this instanceof MyTransform))
  4004      return new MyTransform(options);

yotam707 · 2022-02-18T09:07:32Z

I will take this issue - since it's my first issue to tackle and allow others to contribute as well I will take all the changes in the lib/zlib.js file.

Uzlopak · 2022-02-18T10:27:36Z

Wow. What is this magic?

So if we have a normal function call without new, than the target attribute is not set so we can determine, that is is not instantiated with new?

kurtextrem · 2022-02-18T10:42:28Z

For reference here too: new.target === undefined is faster than !new.target: esbench.com/bench/620f77c86c89f600a570162f

aduh95 · 2022-02-18T10:47:30Z

For reference here too: new.target === undefined is faster than !new.target: esbench.com/bench/620f77c86c89f600a570162f

It's faster on Firefox and Safari, slower on Chromium. Since Node.js uses V8 like Chromium, !new.target should be the correct choice for now.

Chromium:

test	result (higher is better)
explicit undefined	89,842,444 ops/s±0.96%samples
instanceof	17,605,837 ops/s±0.77%samples
!new.target	91,435,665 ops/s±0.84%samples

Firefox:

test	result (higher is better)
explicit undefined	99,491,500 ops/s±1.14% samples
instanceof	6,792,599 ops/s±1.04% samples
!new.target	87,092,012 ops/s±3.70% samples

Safari:

test	result (higher is better)
explicit undefined	957,915,420 ops/s±0.55%samples
instanceof	14,945,959 ops/s±1.12%samples
!new.target	955,919,550 ops/s±0.59%samples

kurtextrem · 2022-02-18T10:51:08Z

V8 10.0.136

aduh95 · 2022-02-18T11:04:53Z

TL;DR: checking new.target === undefined is faster than checking !new.target.

Just tried it with Node.js (because browsers are not relevant in this discussion after all):

const max = 100000000

function InstanceOfClass () {
  if (!(this instanceof InstanceOfClass)) {
    return new InstanceOfClass()
  }
}

function NewTargetFalsyClass () {
  if (!new.target) {
    return new NewTargetFalsyClass()
  }
}

function NewTargetNullishClass () {
  if (new.target == null) {
    return new NewTargetNullishClass()
  }
}

function NewTargetUndefinedClass () {
  if (new.target === undefined) {
    return new NewTargetUndefinedClass()
  }
}

console.time('InstanceOfClass')
for (let i = 0; i < max; i++) {
  InstanceOfClass()
}
console.timeEnd('InstanceOfClass')

console.time('NewTargetFalsyClass')
for (let i = 0; i < max; i++) {
  NewTargetFalsyClass()
}
console.timeEnd('NewTargetFalsyClass')

console.time('NewTargetNullishClass')
for (let i = 0; i < max; i++) {
  NewTargetNullishClass()
}
console.timeEnd('NewTargetNullishClass')

console.time('NewTargetUndefinedClass')
for (let i = 0; i < max; i++) {
  NewTargetUndefinedClass()
}
console.timeEnd('NewTargetUndefinedClass')

Node.js v16.14.0

InstanceOfClass: 5.630s
NewTargetFalsyClass: 1.238s
NewTargetNullishClass: 1.210s
NewTargetUndefinedClass: 1.148s

Node.js v17.5.0

InstanceOfClass: 5.230s
NewTargetFalsyClass: 1.251s
NewTargetNullishClass: 1.262s
NewTargetUndefinedClass: 1.202s

Node.js v18.0.0-pre

InstanceOfClass: 5.305s
NewTargetFalsyClass: 1.155s
NewTargetNullishClass: 1.090s
NewTargetUndefinedClass: 1.069s

It looks like recent V8 upgrades seems to prefer explicit undefined indeed! So I agree, let's use that.

Mesteery · 2022-02-18T13:25:55Z

I don't think it's really possible to use new.target instead of instanceof because it doesn't work with inheritance:

function NewTarget() {
  if (new.target === undefined) {
    return new NewTarget();
  }
  this.value = 1;
}
function FinalClass() {
  NewTarget.call(this)
}
Object.setPrototypeOf(FinalClass.prototype, NewTarget.prototype);
Object.setPrototypeOf(FinalClass, NewTarget);

console.log(new FinalClass().value) // undefined

Refs: #42044 (comment)

Uzlopak · 2022-02-18T13:46:31Z

@mcollina
Your snippet says still // undefined
Just adding that with matteos snippet console.log(new FinalClass().value) returns 1

Mesteery · 2022-02-18T13:49:55Z

Just adding that with matteos snippet console.log(new FinalClass().value) returns 1

Yes, but console.log(NewTarget()) would no longer work (-> undefined).

mcollina · 2022-02-18T13:53:24Z

I think this is a possible solution:

function NewTarget() {
  if (new.target === undefined && this === global) {
    return new NewTarget();
  }
  this.value = 1;
}
function FinalClass() {
  if (new.target === undefined && this === global) {
    return new FinalClass();
  }
  NewTarget.call(this)
}
Object.setPrototypeOf(FinalClass.prototype, NewTarget.prototype);
Object.setPrototypeOf(FinalClass, NewTarget);

console.log(new NewTarget().value)
console.log(new FinalClass().value)
console.log(NewTarget().value)
console.log(FinalClass().value)

(I uploaded a previous snippet that I canceled because it was wrong).

Uzlopak · 2022-02-18T13:55:33Z

Now the question remains: Is this now even faster?

Mesteery · 2022-02-18T13:57:03Z

(Yes, at least on my machine)

InstanceOfClass: 11.456s
NewTargetClass: 2.896s

aduh95 · 2022-02-18T13:57:28Z

new.target === undefined && this === global

Would that work if the function is called from another function scope or a module scope? (btw this could be the only place where globalThis makes more sense than global for the name of the global object)

EDIT: confirmed it doesn't work on module scope:

file://…:5
    this.value = 1;
               ^

TypeError: Cannot set properties of undefined (setting 'value')
    at NewTarget (file://…:5:16)

mcollina · 2022-02-18T14:03:41Z

So it should be globalThis?

aduh95 · 2022-02-18T14:11:33Z

OK this seems to work in both CJS and ESM contexts:

function NewTarget() {
  if (new.target === undefined && (this === globalThis || this == null)) {
    return new NewTarget();
  }
  this.value = 1;
}
function FinalClass() {
  if (new.target === undefined && (this === globalThis || this == null)) {
    return new FinalClass();
  }
  NewTarget.call(this);
}
Object.setPrototypeOf(FinalClass.prototype, NewTarget.prototype);
Object.setPrototypeOf(FinalClass, NewTarget);

console.log(new NewTarget().value);
console.log(new FinalClass().value);
console.log(NewTarget().value);
console.log(FinalClass().value);

I'm not 100% confident that this will not break in another context we haven't think of yet though, it may be prudent to land those changes as semver-major.

targos · 2022-02-18T14:12:52Z

I we end up doing a semver-major change, shouldn't we move to class definitions?

mscdex · 2022-02-18T14:20:41Z

I agree, we really should just move to using class for all of these uses of "instanceof" in constructors at once in a new major version. I'm sure it will cause breakage for some, but the performance benefit is worth it IMO (especially considering the code change needed to fix the breakage should be very minimal).

Uzlopak · 2022-02-18T14:22:56Z

Btw. @jimmywarting benched the performance between classes and prototypes and reported that classes in node are slower, but in deno there is not this performance penalty. So shouldnt we first improve the perf hit by using classes before forcing everybody using classes?

aduh95 · 2022-02-18T14:24:52Z

The breakage would be quite major though, for any user-library out there who is written in ES5 syntax and inherits from one of Node.js's core classes would need to refactor.

mcollina · 2022-02-18T14:57:22Z

I propose we we should:

perform the change in Move from instanceof to new.target check in all function-based classes #42042 (comment) to events, http & streams
benchmark the results & validate the expected improvement
perform a CITGM
release in v17 but not backport it to v16 for safety.

bmeck · 2022-02-18T16:47:03Z

function NewTarget() {
  if (new.target === undefined && (this === globalThis || this == null)) {
    return new NewTarget();
  }
  this.value = 1;
}

this looks incorrect, it shouldn't be doing a this check. What exactly is the this check trying to do?

See:

function NewTarget() {
  if (new.target === undefined && (this === globalThis || this == null)) {
    return new NewTarget();
  }
  this.value = 1;
}
let http = {
  NewTarget
};
http.NewTarget();
console.dir(http); // http.value === 1

aduh95 · 2022-02-18T16:54:57Z

Back to the original idea then, we need an instanceof check.

const max = 100000000

function InstanceOfClass () {
  if (!(this instanceof InstanceOfClass)) {
    return new InstanceOfClass()
  }
}

function NewTargetAndInstanceOfClass () {
  if (new.target === undefined && !(this instanceof NewTarget)) {
    return new NewTargetAndInstanceOfClass()
  }
}

console.time('InstanceOfClass')
for (let i = 0; i < max; i++) {
  new InstanceOfClass()
}
console.timeEnd('InstanceOfClass')

console.time('NewTargetAndInstanceOfClass')
for (let i = 0; i < max; i++) {
  new NewTargetAndInstanceOfClass()
}
console.timeEnd('NewTargetAndInstanceOfClass')

It still improves perf when using the new keyboard, so it may may still be worth it.

Node.js v16.14.0
InstanceOfClass: 774.66ms
NewTargetAndInstanceOfClass: 57.237ms

Node.js v17.5.0
InstanceOfClass: 699.107ms
NewTargetAndInstanceOf: 55.764ms

Node.js v18.0.0-pre
InstanceOfClass: 621.932ms
NewTargetAndInstanceOfClass: 56.512ms

bmeck · 2022-02-18T17:12:52Z

that seems fine

mcollina · 2022-02-18T17:34:20Z

Here is something incredibly funny:

const max = 100000000

function InstanceOfClass () {
  if (!(this instanceof InstanceOfClass)) {
    return new InstanceOfClass()
  }
  this.value = 0
}

function NewTargetAndInstanceOfClass () {
  if (new.target === undefined && !(this instanceof NewTargetAndInstanceOfClass)) {
    return new NewTargetAndInstanceOfClass()
  }
  this.value = 0
}

console.time('InstanceOfClass')
for (let i = 0; i < max; i++) {
  new InstanceOfClass()
}
console.timeEnd('InstanceOfClass')

console.time('NewTargetAndInstanceOfClass')
for (let i = 0; i < max; i++) {
  new NewTargetAndInstanceOfClass()
}
console.timeEnd('NewTargetAndInstanceOfClass')

leads to:

InstanceOfClass: 92.097ms
NewTargetAndInstanceOfClass: 91.78ms

As soon as we add a property we hit a fast-path in V8 that removes any difference in speed.

This was not a great idea after all.

Uzlopak · 2022-02-18T17:42:03Z

So we should use this, if we have empty Objects. ;)

Qard · 2022-02-18T18:05:54Z

I love that every time people try to outsmart V8 it proves to be smarter than people expect. 😆

targos · 2022-02-18T18:30:21Z

As soon as we add a property we hit a fast-path in V8 that removes any difference in speed.

So we should use this, if we have empty Objects. ;)

Wow. Let's add "and remove all class fields" to this issue. As a bonus, it should also help with the memory footprint 😄

mcollina added the feature request Issues that request new features to be added to Node.js. label Feb 18, 2022

lpinca added the performance Issues and PRs related to the performance of Node.js. label Feb 18, 2022

yotam707 mentioned this issue Feb 18, 2022

zlib: move from instanceof check to new.target check #42044

Closed

ronag mentioned this issue Feb 18, 2022

Move from instanceof to new.target check in all function-based classes nodejs/undici#1241

Closed

mcollina added the good first issue Issues that are suitable for first-time contributors. label Feb 18, 2022

mcollina removed the good first issue Issues that are suitable for first-time contributors. label Feb 18, 2022

mcollina closed this as completed Feb 18, 2022

Uzlopak mentioned this issue Feb 19, 2022

Revert "performance patch" fastify/fastify-error#61

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from instanceof to new.target check in all function-based classes #42042

Move from instanceof to new.target check in all function-based classes #42042

mcollina commented Feb 18, 2022 •

edited

benjamingr commented Feb 18, 2022 •

edited

yotam707 commented Feb 18, 2022 •

edited

Uzlopak commented Feb 18, 2022

kurtextrem commented Feb 18, 2022

aduh95 commented Feb 18, 2022 •

edited

kurtextrem commented Feb 18, 2022

aduh95 commented Feb 18, 2022 •

edited

Mesteery commented Feb 18, 2022 •

edited

Uzlopak commented Feb 18, 2022

Mesteery commented Feb 18, 2022 •

edited

mcollina commented Feb 18, 2022

Uzlopak commented Feb 18, 2022

Mesteery commented Feb 18, 2022 •

edited

aduh95 commented Feb 18, 2022 •

edited

mcollina commented Feb 18, 2022

aduh95 commented Feb 18, 2022

targos commented Feb 18, 2022

mscdex commented Feb 18, 2022 •

edited

Uzlopak commented Feb 18, 2022 •

edited

aduh95 commented Feb 18, 2022

mcollina commented Feb 18, 2022

bmeck commented Feb 18, 2022

aduh95 commented Feb 18, 2022

bmeck commented Feb 18, 2022

mcollina commented Feb 18, 2022

Uzlopak commented Feb 18, 2022

Qard commented Feb 18, 2022

targos commented Feb 18, 2022

Move from instanceof to new.target check in all function-based classes #42042

Move from instanceof to new.target check in all function-based classes #42042

Comments

mcollina commented Feb 18, 2022 • edited

What is the problem this feature will solve?

What is the feature you are proposing to solve the problem?

What alternatives have you considered?

benjamingr commented Feb 18, 2022 • edited

yotam707 commented Feb 18, 2022 • edited

Uzlopak commented Feb 18, 2022

kurtextrem commented Feb 18, 2022

aduh95 commented Feb 18, 2022 • edited

kurtextrem commented Feb 18, 2022

aduh95 commented Feb 18, 2022 • edited

Mesteery commented Feb 18, 2022 • edited

Uzlopak commented Feb 18, 2022

Mesteery commented Feb 18, 2022 • edited

mcollina commented Feb 18, 2022

Uzlopak commented Feb 18, 2022

Mesteery commented Feb 18, 2022 • edited

aduh95 commented Feb 18, 2022 • edited

mcollina commented Feb 18, 2022

aduh95 commented Feb 18, 2022

targos commented Feb 18, 2022

mscdex commented Feb 18, 2022 • edited

Uzlopak commented Feb 18, 2022 • edited

aduh95 commented Feb 18, 2022

mcollina commented Feb 18, 2022

bmeck commented Feb 18, 2022

aduh95 commented Feb 18, 2022

bmeck commented Feb 18, 2022

mcollina commented Feb 18, 2022

Uzlopak commented Feb 18, 2022

Qard commented Feb 18, 2022

targos commented Feb 18, 2022

mcollina commented Feb 18, 2022 •

edited

benjamingr commented Feb 18, 2022 •

edited

yotam707 commented Feb 18, 2022 •

edited

aduh95 commented Feb 18, 2022 •

edited

aduh95 commented Feb 18, 2022 •

edited

Mesteery commented Feb 18, 2022 •

edited

Mesteery commented Feb 18, 2022 •

edited

Mesteery commented Feb 18, 2022 •

edited

aduh95 commented Feb 18, 2022 •

edited

mscdex commented Feb 18, 2022 •

edited

Uzlopak commented Feb 18, 2022 •

edited