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

Unexpected performance penalty when using field initialiser #33698

Closed
pbadenski opened this issue Jun 2, 2020 · 5 comments
Closed

Unexpected performance penalty when using field initialiser #33698

pbadenski opened this issue Jun 2, 2020 · 5 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@pbadenski
Copy link

pbadenski commented Jun 2, 2020

I'm getting 2 orders of magnitude performance penalty when using field initialiser. See snippet below (corrected after @devsnek comment):

"use strict";

class A_field_initialized_in_constructor {
  constructor() {
    this.foo = true;
  }

}

class B_field_initialized_as_class_property {
  foo = true;
}

and the results (using https://www.npmjs.com/package/benchmark):

A_field_initialized_in_constructor x 812,776,791 ops/sec ±0.94% (91 runs sampled)
B_field_initialized_as_class_property x 5,935,129 ops/sec ±2.31% (86 runs sampled)

Environment:

> uname -a
Darwin Pawels-MBP-3.lan 19.4.0 Darwin Kernel Version 19.4.0: Wed Mar  4 22:28:40 PST 2020; root:xnu-6153.101.6~15/RELEASE_X86_64 x86_64
> node -v
v14.3.0
@devsnek
Copy link
Member

devsnek commented Jun 2, 2020

Can you post a reproduction that uses only pure JavaScript?

@devsnek devsnek added the v8 engine Issues and PRs related to the V8 dependency. label Jun 2, 2020
@devsnek
Copy link
Member

devsnek commented Jun 2, 2020

@pbadenski thanks, i would recommend reporting this to https://bugs.chromium.org/p/v8/issues/list

@lpinca
Copy link
Member

lpinca commented Jun 2, 2020

A_field_initialized_in_constructor x 812,776,791 ops/sec ±0.94% (91 runs sampled)

I'm pretty sure this is optimized into a noop. Try to do something like this

let instance;

suite.add('A_field_initialized_in_constructor', function() {
  instance = new A_field_initialized_in_constructor();
  return instance;
});

suite.add('B_field_initialized_as_class_property', function() {
  instance = new B_field_initialized_as_class_property();
  return instance;
});

and see if it changes the results.

@pbadenski
Copy link
Author

pbadenski commented Jun 3, 2020

@lpinca I followed the suggestion - the difference reduced to one order of magnitude, but still surprisingly large:

A_field_initialized_in_constructor x 77,519,027 ops/sec ±1.85% (90 runs sampled)
B_field_initialized_as_class_property x 6,270,322 ops/sec ±1.21% (88 runs sampled)

Test:

const { Options, Suite } = require("benchmark");

const runBenchmarkSuite = (suite) => {
  return new Promise((resolve, reject) => {
    suite
      .on("start", (e) => console.log(`Running test: ${e.currentTarget.name}`))
      .on("cycle", (e) => console.log(String(e.target)))
      .on("complete", (e) => console.log(`Fastest is ${e.currentTarget.filter("fastest").map("name")}`))
      .on("complete", resolve)
      .on("error", reject)
      .run();
  });
};

class A_field_initialized_in_constructor {
  constructor() {
    this.foo = true;
  }

}

class B_field_initialized_as_class_property {
  foo = true;
}

let instance;
runBenchmarkSuite(new Suite("fields")
  .add("A_field_initialized_in_constructor", () => {
    instance = new A_field_initialized_in_constructor();
    return instance;
  })
  .add("B_field_initialized_as_class_property", () => {
    instance = new B_field_initialized_as_class_property();
    return instance;
  })
);

I'll report to v8.

@targos
Copy link
Member

targos commented Nov 20, 2021

Closing as this should be fixed in V8. Feel free to post a link to the V8 issue if it exists.

@targos targos closed this as completed Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

4 participants