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

Revert "timers: refactor to use optional chaining" #38245

Closed

Conversation

mcollina
Copy link
Member

This reverts commit d8f535b.

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

With optional chaining:

Screenshot 2021-04-15 at 12 07 40

Without optional chanining:

Screenshot 2021-04-15 at 12 07 51

This sits in the hot path for HTTP.


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

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Apr 15, 2021
@mcollina mcollina requested a review from ronag April 15, 2021 10:15
@Flarna
Copy link
Member

Flarna commented Apr 15, 2021

Should we report this towards v8?

@ronag
Copy link
Member

ronag commented Apr 15, 2021

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

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

LGTM, gj

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member Author

cc @nodejs/tsc

@targos
Copy link
Member

targos commented Apr 15, 2021

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
Copy link
Member

ronag commented Apr 15, 2021

This reverts commit d8f535b.

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

With optional chaining:

Screenshot 2021-04-15 at 12 07 40

Without optional chanining:

Screenshot 2021-04-15 at 12 07 51

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
Copy link
Member Author

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
Copy link
Member

targos commented Apr 15, 2021

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Apr 16, 2021

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>}

@jasnell
Copy link
Member

jasnell commented Apr 16, 2021

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 19, 2021

@mcollina
Copy link
Member Author

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

@BethGriggs
Copy link
Member

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

@BethGriggs
Copy link
Member

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 pushed a commit that referenced this pull request Apr 19, 2021
This reverts commit d8f535b.

PR-URL: #38245
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs
Copy link
Member

Landed in a0261d2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.