Should we report this towards v8?

Yea this seems like a V8 issue. @nodejs/v8

CI: https://ci.nodejs.org/job/node-test-pull-request/37487/

cc @nodejs/tsc

I wanted to prove it with a benchmark and open a V8 issue but it seems optional chaining is mostly faster (at least with V8 8.9): https://jsben.ch/BwkJK

This reverts commit d8f535b.

As part of #37937, I tracked down a regression introduced by #36767.

With optional chaining:

Without optional chanining:

This sits in the hot path for HTTP.

I will recommend caution in adopting optional chaining in other areas of the Node.js.

@mcollina: What node version were you trying this on?

I wanted to prove it with a benchmark and open a V8 issue but it seems optional chaining is mostly faster (at least with V8 8.9): https://jsben.ch/BwkJK

I have literally no clue why, but without optional chaining those function calls get inlined.

Those flamegraphs come from master.

those function calls get inlined.

That's a good point, thanks. I'll try to do a different benchmark to show that optional chaining prevents inlining.

CI: https://ci.nodejs.org/job/node-test-pull-request/37491/

CI: https://ci.nodejs.org/job/node-test-pull-request/37509/

those function calls get inlined.

That's a good point, thanks. I'll try to do a different benchmark to show that optional chaining prevents inlining.

It's not that easy. Everything gets inlined with this example:

'use strict';

function getPropClassic(obj) {
  return obj && obj.prop;
}

function getPropOptional(obj) {
  return obj?.prop;
}

const obj = { prop: 42 };

function testGetPropClassic(obj) {
  return getPropClassic(obj);
}

function testGetPropOptional(obj) {
  return getPropOptional(obj);
}

for (let i = 0; i < 1e6; i++) {
  testGetPropClassic(obj);
  testGetPropOptional(obj);
}

> node --trace-turbo-inlining bench.js
Considering 000002D5A0AE59D8 {0x004b3832c8e1 <SharedFunctionInfo testGetPropClassic>} for inlining with 000002D5A0AE59E8 {0x004b3832d821 <FeedbackVector[2]>}
Inlining small function(s) at call site #45:JSCall
Inlining 000002D5A0AE59D8 {0x004b3832c8e1 <SharedFunctionInfo testGetPropClassic>} into 000002D5A0AE4A98 {0x004b3832c779 <SharedFunctionInfo>}
Considering 000002D5A0AE63A8 {0x004b3832c841 <SharedFunctionInfo getPropClassic>} for inlining with 000002D5A0AE63B8 {0x004b3832d7e1 <FeedbackVector[2]>}
Inlining small function(s) at call site #80:JSCall
Inlining 000002D5A0AE63A8 {0x004b3832c841 <SharedFunctionInfo getPropClassic>} into 000002D5A0AE4A98 {0x004b3832c779 <SharedFunctionInfo>}
Considering 000002D5A0AE69E8 {0x004b3832c931 <SharedFunctionInfo testGetPropOptional>} for inlining with 000002D5A0AE6DF8 {0x004b3832d861 <FeedbackVector[2]>}
Inlining small function(s) at call site #50:JSCall
Inlining 000002D5A0AE69E8 {0x004b3832c931 <SharedFunctionInfo testGetPropOptional>} into 000002D5A0AE4A98 {0x004b3832c779 <SharedFunctionInfo>}
Considering 000002D5A0AE78A8 {0x004b3832c891 <SharedFunctionInfo getPropOptional>} for inlining with 000002D5A0AE78B8 {0x004b3832d7a1 <FeedbackVector[2]>}
Inlining small function(s) at call site #129:JSCall
Inlining 000002D5A0AE78A8 {0x004b3832c891 <SharedFunctionInfo getPropOptional>} into 000002D5A0AE4A98 {0x004b3832c779 <SharedFunctionInfo>}

@targos I'm curious... Can you try that example but with obj replaced with a class with a getter?

class Obj { get prop() { return 42; }  }
const obj = new Obj()

Also, let's see what happens when you alternate calls between the argument being obj and undefined.

CI: https://ci.nodejs.org/job/node-test-pull-request/37562/ ✅

"Build from tarball / test-tarball-linux (pull_request) " keeps failing. Is that required to pass for this landing? cc @BethGriggs

Failure is js-native-api/test_object/test, which was mentioned as being fixed by #38000. Rerunning as that has just landed 🤞🏻

I hadn't appreciated before that GH actions do not rebase. fc20e83 was the fix for the failure seen here (js-native-api/test_object/test). I think we should go ahead and land rather than rebase and rerun (due to the timing).

Landed in a0261d2