Revert "timers: refactor to use optional chaining"

[mcollina]

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.

[Flarna]

Should we report this towards v8?

[ronag]

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

[ronag]

LGTM, gj

[nodejs-github-bot]

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

[mcollina]

cc @nodejs/tsc

[targos]

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

[ronag]

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?

[mcollina]

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.

[targos]

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.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[targos]

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';functiongetPropClassic(obj){returnobj&&obj.prop;}functiongetPropOptional(obj){returnobj?.prop;}constobj={prop: 42};functiontestGetPropClassic(obj){returngetPropClassic(obj);}functiontestGetPropOptional(obj){returngetPropOptional(obj);}for(leti=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>} 

[jasnell]

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

classObj{getprop(){return42;}}constobj=newObj()

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

[mhdawson]

LGTM

[nodejs-github-bot]

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

[mcollina]

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

[BethGriggs]

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

[BethGriggs]

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).

[BethGriggs]

Landed in a0261d2