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

tools: add prefer-proto rule #46083

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Jan 3, 2023

If this lands, I'd be happy to refactor this rule to a prefer-syntax rule, which also checks for things like ObjectAssign with a literal as the first argument, use of Boolean(x) instead of !!x, etc.

fixup: add support for `Object.create(null)`

fixup: extend to any 1-argument Object.create call

fixup: add tests
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/http2
  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/net
  • @nodejs/startup
  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 3, 2023
@ljharb
Copy link
Member Author

ljharb commented Jan 3, 2023

cc @aduh95

@cjihrig
Copy link
Contributor

cjihrig commented Jan 3, 2023

I'm in favor of having more consistency here. I have a couple of questions:

  1. Can we remove all ObjectCreate() occurrences from the codebase? I noticed the instance from tools: add prefer-proto rule #46083 (review) is no longer part of this diff because it has more than one argument.
  2. If autofixing with the second argument is off the table (current state of this PR), could we still show a lint error?

@ljharb
Copy link
Member Author

ljharb commented Jan 3, 2023

The alternative form of ObjectCreate(null, x) is ObjectDefineProperties({ __proto__: null, x) which i'm not sure is better. There's also at least one ObjectCreate(constructor, x) form.

I am more than happy to do a more thorough audit once there's general agreement on the lint rule's philosophy (prefer syntax over API)

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'm probably less opinionated about this than some other folks 😄

.eslintrc.js Show resolved Hide resolved
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm when the tests succeed

@ljharb ljharb force-pushed the prefer-proto branch 2 times, most recently from eb9100e to 06b629e Compare January 4, 2023 05:56
@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2023
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
fixup: add support for `Object.create(null)`

fixup: extend to any 1-argument Object.create call

fixup: add tests
PR-URL: nodejs#46083
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS
Copy link
Member

@ljharb This commit didn't land cleanly on v19.x because some branches contain the ObjectCreate. I'll open a backport for you.

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
fixup: add support for `Object.create(null)`

fixup: extend to any 1-argument Object.create call

fixup: add tests
PR-URL: nodejs#46083
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS
Copy link
Member

@ljharb here you go #46239.

@Qard
Copy link
Member

Qard commented Jan 19, 2023

Is there some explanation for why this change happened? Nothing against it, just could use some explanation in the PR description so people know why we prefer the syntax form over the function call form.

@ljharb
Copy link
Member Author

ljharb commented Jan 19, 2023

@Qard sure, i'd phrase it as:

Syntax is always preferred over equivalent non-syntax forms, modulo readability concerns, and __proto__ syntax is simpler, faster, and most importantly, more idiomatic than a primordial call. Implied here is also that a safe idiomatic approach would always be preferred over an equivalent primordial approach.

Does that suffice?

@Qard
Copy link
Member

Qard commented Jan 19, 2023

Sounds reasonable, thanks! 🙂

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
fixup: add support for `Object.create(null)`

fixup: extend to any 1-argument Object.create call

fixup: add tests
PR-URL: #46083
Backport-PR-URL: #46239
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
@WebReflection
Copy link
Contributor

WebReflection commented Jan 20, 2023

@ljharb about this:

and __proto__ syntax is simpler, faster

I have very mixed results across gjs, bun, or node ... not the best benchmark ever, but __proto__ is not always the best option. What happened to Null class? how come node doesn't have one in core? 'cause I remember this was already tested and benchmarked as the best option.

Here a test example:

const {assign, create, setPrototypeOf} = Object;

class Test {
  constructor(i) {
    this.i = i;
  }
}

class CEWValue {
  constructor(value) {
    this.value = value;
  }
  get configurable() { return true }
  get enumerable() { return true }
  get writable() { return true }
}

class Null {}
setPrototypeOf(
  Null.prototype,
  null
);

const bench = {
  ["__proto__"](times, withField) {
    const instances = [];
    for (let {prototype} = Test, i = 0; i < times; i++)
      instances[i] = withField ?
        {__proto__: prototype, i: Math.random()} :
        {__proto__: prototype};
    return instances;
  },
  create(times, withField) {
    const instances = [];
    for (let {prototype} = Test, i = 0; i < times; i++)
      instances[i] = withField ?
        create(prototype, {"i": new CEWValue(Math.random())}) :
        create(prototype);
    return instances;
  },
  new(times, withField) {
    const instances = [];
    for (let i = 0; i < times; i++)
      instances[i] = withField ?
        new Test(Math.random()) :
        new Test;
    return instances;
  },
  Null(times, withField) {
    const instances = [];
    for (let i = 0; i < times; i++)
      instances[i] = withField ?
        assign(new Null, {i: Math.random()}) :
        new Null;
    return instances;
  }
};

function test(name, times, Class = Test) {
  if (typeof process === 'object')
    console.log(`\x1b[1m${name}\x1b[0m`);
  else
    console.log(`%c${name}`, 'font-weight:bold');
  let instances = [new Class(0)];
  console.time('cold - no fields');
  instances = bench[name](times, false);
  console.timeEnd('cold - no fields');
  for (let i = 0; i < 5; i++)
    instances = bench[name](times, false);
  console.time('hot - no fields');
  instances = bench[name](times, false);
  console.timeEnd('hot - no fields');
  console.time('cold ? fields');
  instances = bench[name](times, true);
  console.timeEnd('cold ? fields');
  for (let i = 0; i < 5; i++)
    instances = bench[name](times, true);
  console.time('hot - fields');
  instances = bench[name](times, true);
  console.timeEnd('hot - fields');
  verify(name, instances, Class);
  console.log('');
}

function verify(name, instances, Class) {
  console.assert(
    instances.every(o => o instanceof Class && typeof o.i === 'number'),
    `${name} failed`
  );
}

function benchAll(times) {
  test("new", times);
  test("Null", times, Null);
  test("create", times);
  test("__proto__", times);
}

benchAll(10_000);

@ljharb
Copy link
Member Author

ljharb commented Jan 20, 2023

@WebReflection if { __proto__: Null } is faster than { __proto__: null } then i'm sure we'd all be interested in that! My claim was that { __proto__: null } was faster than ObjectCreate(null).

@WebReflection
Copy link
Contributor

@ljharb it's new Null vs {__proto__: null} but definitively everything is faster than Object.create

@juanarbol juanarbol added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jan 25, 2023
@juanarbol
Copy link
Member

This does not land cleanly on v18.x

@ljharb
Copy link
Member Author

ljharb commented Jan 25, 2023

@juanarbol thanks, does #46239 land cleanly? if not, ill try to open a backport PR.

@maximelkin
Copy link

Patch to benchmark:

-        create(prototype, {"i": new CEWValue(Math.random())})
+        assign(create(prototype), {"i": Math.random()}) :
after patch

const {assign, create, setPrototypeOf} = Object;

class Test {
  constructor(i) {
    this.i = i;
  }
}

class CEWValue {
  constructor(value) {
    this.value = value;
  }
  get configurable() { return true }
  get enumerable() { return true }
  get writable() { return true }
}

class Null {}
setPrototypeOf(
  Null.prototype,
  null
);

const bench = {
  ["__proto__"](times, withField) {
    const instances = [];
    for (let {prototype} = Test, i = 0; i < times; i++)
      instances[i] = withField ?
        {__proto__: prototype, i: Math.random()} :
        {__proto__: prototype};
    return instances;
  },
  create(times, withField) {
    const instances = [];
    for (let {prototype} = Test, i = 0; i < times; i++)
      instances[i] = withField ?
        assign(create(prototype), {"i": Math.random()}) :
        create(prototype);
    return instances;
  },
  new(times, withField) {
    const instances = [];
    for (let i = 0; i < times; i++)
      instances[i] = withField ?
        new Test(Math.random()) :
        new Test;
    return instances;
  },
  Null(times, withField) {
    const instances = [];
    for (let i = 0; i < times; i++)
      instances[i] = withField ?
        assign(new Null, {i: Math.random()}) :
        new Null;
    return instances;
  }
};

function test(name, times, Class = Test) {
  if (typeof process === 'object')
    console.log(`\x1b[1m${name}\x1b[0m`);
  else
    console.log(`%c${name}`, 'font-weight:bold');
  let instances = [new Class(0)];
  console.time('cold - no fields');
  instances = bench[name](times, false);
  console.timeEnd('cold - no fields');
  for (let i = 0; i < 5; i++)
    instances = bench[name](times, false);
  console.time('hot - no fields');
  instances = bench[name](times, false);
  console.timeEnd('hot - no fields');
  console.time('cold ? fields');
  instances = bench[name](times, true);
  console.timeEnd('cold ? fields');
  for (let i = 0; i < 5; i++)
    instances = bench[name](times, true);
  console.time('hot - fields');
  instances = bench[name](times, true);
  console.timeEnd('hot - fields');
  verify(name, instances, Class);
  console.log('');
}

function verify(name, instances, Class) {
  console.assert(
    instances.every(o => o instanceof Class && typeof o.i === 'number'),
    `${name} failed`
  );
}

function benchAll(times) {
  test("new", times);
  test("Null", times, Null);
  test("create", times);
  test("__proto__", times);
}

benchAll(10_000);

node v19.5.0, ubuntu 20.04

new
cold - no fields: 1.156ms
hot - no fields: 0.326ms
cold ? fields: 1.486ms
hot - fields: 1.193ms

Null
cold - no fields: 0.821ms
hot - no fields: 0.059ms
cold ? fields: 1.275ms
hot - fields: 0.987ms

create
cold - no fields: 0.798ms
hot - no fields: 0.262ms
cold ? fields: 1.744ms
hot - fields: 0.548ms

__proto__
cold - no fields: 1.124ms
hot - no fields: 0.529ms
cold ? fields: 2.146ms
hot - fields: 1.545ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.