benchmark: use object literal shorthands

[vsemozhetbyt]

Checklist

• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

benchmark

For extra precaution, a benchmark to compare full and short notation is also added. It shows no significant degradation, if any could be with this change.

[vsemozhetbyt]

Linter CI: https://ci.nodejs.org/job/node-test-linter/7994/

[lpinca]

I'm not against this change but it adds little value imho.

[vsemozhetbyt]

@lpinca Well, I thought it increased readability and maintenanceability a little (decreased tautology, redundancy, syntactical noise). And I've started from benchmark to see how it goes. In tests there are much more cases, maybe hundreds. If it is worthless change, then I shall not start with tests.

[lpinca]

@vsemozhetbyt wait for other collaborators opinions. Again, I'm not against these changes but in the past we avoided making purely cosmetic changes.

[vsemozhetbyt]

FWIW, just some examples from tests with rewritten variants:

  const pair = {
    server: server,
    client: client,
  };

  const pair = { server, client };

---

  child = spawn('/usr/bin/env', [], {env: env});

  child = spawn('/usr/bin/env', [], { env });

---

      process.send({received: received});

      process.send({ received });

---

  result = fs.realpathSync(string_dir, {encoding: encoding});

  result = fs.realpathSync(string_dir, { encoding });

---

    const options = {
      port: port,
      host: host,
      rejectUnauthorized: false,
      _agentKey: agent.getName({
        port: port,
        host: host,
      }),
    };

    const options = {
      port,
      host,
      rejectUnauthorized: false,
      _agentKey: agent.getName({ port, host }),
    };

---

  const options = {
    port: port,
    path: path,
    ca: ca
  };

  const options = { port, path, ca };

[gibfahn]

I think the change improves readability, so +1 from me. Adding a benchmark is also good.

Don't really have the expertise to approve.

cc/ @nodejs/benchmarking @mscdex

Is @nodejs/benchmarking the team we should cc for performance things? I see them in onboarding-extras, but that doesn't include people I've seen be active in recent performance issues (like @mscdex and @joyeecheung).

[sam-github]

I think examples like const options = { port, path, ca }; are nice, but that actual PR has a bunch of changes like:

{
  key: someValue,
  other: 9,
  blah,  // used to be blah: blah,
  someKey: 12
}

And that is not more readable, because it mixes syntax sugar and non-sugar in a single literal, so I have to shift my thinking back and forth. I'm not a fan of that.

[joyeecheung]

@gibfahn I think the benchmarking team is more about https://benchmarking.nodejs.org/? (We could make another team for core-related issues tough, e.g. nodejs/performance...)

I am +0 on making function shorthands, but yeah mixing literal shorthands with normal key-value pairs is a bit confusing like what #12194 (comment) says.

[jasnell]

Historically, we have avoided modifying existing benchmarks too much to ensure that they still have a reasonable chance of running in older versions of Node.js. That said, I do not believe any of the suggested changes are problematic for Node.js 4.x and on.

[vsemozhetbyt]

Maybe mixing { prop } and { prop: prop, prop2: something } is a bit confusing approach too.

Well, it seems to be a rather controversial change. I shall close for now. Thank you for the feedback)