GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||
GeezerVison is showing github.com — Revert "timers: refactor to use optional chaining" by mcollina · Pull Request #38245 · nodejs/node ConversationThis reverts commit d8f535b.
2 of 3 tasks
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Flarna
approved these changes
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
ronag
approved these changes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
richardlau
approved these changes
aduh95
approved these changes
targos
approved these changes
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
|
There was an error while loading. Please reload this page.
@mcollina: What node version were you trying this on? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I have literally no clue why, but without optional chaining those function calls get inlined. Those flamegraphs come from master. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
That's a good point, thanks. I'll try to do a different benchmark to show that optional chaining prevents inlining. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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); } |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/37562/ ✅ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
"Build from tarball / test-tarball-linux (pull_request) " keeps failing. Is that required to pass for this landing? cc @BethGriggs |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Failure is |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I hadn't appreciated before that GH actions do not rebase. fc20e83 was the fix for the failure seen here ( |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
|
Landed in a0261d2 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.