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

lib: restate the code to use primordials #36294

Closed
wants to merge 2 commits into from

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.

@@ -121,7 +125,7 @@ function hasUncaughtExceptionCaptureCallback() {
return exceptionHandlerState.captureFn !== null;
}

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.

None yet

5 participants