Skip to content

Conversation

PoojaDurgad
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. process Issues and PRs related to the process subsystem. labels Nov 27, 2020
@aduh95
Copy link
Contributor

aduh95 commented Nov 27, 2020

The _http_common.js changes are already included in #36194.

}

function noop() {}
const noop = FunctionPrototype;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this change was made. Changing this from a function to a prototype object is changing the type entirely. Additionally, a function itself shouldn't ever need to be converted to any kind of primordial.

Copy link
Contributor

Choose a reason for hiding this comment

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

TBF typeof Function.prototype === 'function', so it's not really changing the type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Even so, I don't see the benefit of changing this unless someone can point to something specifically that this would be protecting us from?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://tc39.es/ecma262/#sec-properties-of-the-function-prototype-object

The Function prototype object:

  • is itself a built-in function object.
  • accepts any arguments and returns undefined when invoked.

It seems to be strictly equivalent to function noop(){}, the only difference I can think of is that it doesn't create a new Function object and may be marginally more memory efficient.
I highly doubt there is any scenario where it would show that Node is using one or the other, so I'm neutral to this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it’s passed to setImmediate, which is obtained from require('timers') at the time the callback created by createOnGlobalUncaughtException is invoked, so if user code wraps setImmediate from timers, it can determine whether the passed function is %Function.prototype%.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mscdex ,@aduh95 - I made the changes based on this logic typeof Function.prototype === 'function' . Hence I modified the code. I am fine to revert this change, if others think it is not worth

@aduh95
Copy link
Contributor

aduh95 commented May 15, 2021

Closing based on performance concerns outlined in #38248.

@aduh95 aduh95 closed this May 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. process Issues and PRs related to the process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants