GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Revert "timers: refactor to use optional chaining" by mcollina · Pull Request #38245 · nodejs/node

Conversation

@mcollina

@mcollina mcollina commented Apr 15, 2021

Copy link
Copy Markdown
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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji ExE-Boss reacted with confused emoji
All reactions
  • 👍 1 reaction
  • 😕 1 reaction
@mcollina
Revert "timers: refactor to use optional chaining"
c87d636
This reverts commit d8f535b.
@mcollina
mcollina requested review from Lxxyx, aduh95 and jasnell April 15, 2021 10:14
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. timers Issues and PRs related to timers, setImmediate(), setInterval(), and setTimeout(). labels Apr 15, 2021
@mcollina
mcollina requested a review from ronag April 15, 2021 10:15
@mcollina mcollina mentioned this pull request Apr 15, 2021
http: significant performance regression on master #37937
Closed
2 of 3 tasks
@Flarna

Flarna commented Apr 15, 2021

Copy link
Copy Markdown
Member

Should we report this towards v8?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Flarna
Flarna approved these changes Apr 15, 2021
View reviewed changes
@ronag

ronag commented Apr 15, 2021

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ronag
ronag approved these changes Apr 15, 2021
View reviewed changes

@ronag ronag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, gj

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Apr 15, 2021

Copy link
Copy Markdown
Member Author

cc @nodejs/tsc

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

richardlau
richardlau approved these changes Apr 15, 2021
View reviewed changes
aduh95
aduh95 approved these changes Apr 15, 2021
View reviewed changes
targos
targos approved these changes Apr 15, 2021
View reviewed changes
@targos

targos commented Apr 15, 2021

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ronag

ronag commented Apr 15, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
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.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ronag ronag mentioned this pull request Apr 15, 2021
2021-04-20, Version 16.0.0 (Current) #37678
Merged
Lxxyx
Lxxyx approved these changes Apr 15, 2021
View reviewed changes
@mcollina

mcollina commented Apr 15, 2021

Copy link
Copy Markdown
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.

ronag reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented Apr 15, 2021

Copy link
Copy Markdown
Member

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.

ronag and mcollina reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BethGriggs
BethGriggs approved these changes Apr 15, 2021
View reviewed changes
cjihrig
cjihrig approved these changes Apr 15, 2021
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell approved these changes Apr 15, 2021
View reviewed changes
lpinca
lpinca approved these changes Apr 15, 2021
View reviewed changes
VoltrexKeyva
VoltrexKeyva approved these changes Apr 15, 2021
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Apr 16, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented Apr 16, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Apr 16, 2021

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhdawson
mhdawson approved these changes Apr 16, 2021
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Trott
Trott approved these changes Apr 17, 2021
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Apr 19, 2021 •
edited by BethGriggs
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Apr 19, 2021

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BethGriggs

BethGriggs commented Apr 19, 2021

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BethGriggs

BethGriggs commented Apr 19, 2021

Copy link
Copy Markdown
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).

aduh95 and mcollina reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BethGriggs pushed a commit that referenced this pull request Apr 19, 2021
@mcollina @BethGriggs
Revert "timers: refactor to use optional chaining"
a0261d2
This reverts commit d8f535b.

PR-URL: #38245
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BethGriggs

BethGriggs commented Apr 19, 2021

Copy link
Copy Markdown
Member

Landed in a0261d2

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BethGriggs BethGriggs closed this Apr 19, 2021
@aduh95 aduh95 mentioned this pull request May 16, 2021
lib: refactor to use optional chaining #38697
Closed
@targos targos added the dont-land-on-v14.x label May 30, 2021
@mcollina mcollina mentioned this pull request Jun 8, 2021
v14.17.1 release proposal #38948
Merged
@aduh95 aduh95 mentioned this pull request Jun 27, 2021
readline: use logical assignment operators #39174
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
@lpinca lpinca lpinca approved these changes
@cjihrig cjihrig cjihrig approved these changes
@ronag ronag ronag approved these changes
@richardlau richardlau richardlau approved these changes
@BethGriggs BethGriggs BethGriggs approved these changes
@Lxxyx Lxxyx Lxxyx approved these changes
@aduh95 aduh95 aduh95 approved these changes
@Flarna Flarna Flarna approved these changes
+3 more reviewers
@targos targos targos approved these changes
@mhdawson mhdawson mhdawson approved these changes
@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. timers Issues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

15 participants

@mcollina @Flarna @ronag @nodejs-github-bot @targos @jasnell @BethGriggs @Trott @lpinca @cjihrig @richardlau @mhdawson @Lxxyx @aduh95 @VoltrexKeyva
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.