-
Notifications
You must be signed in to change notification settings - Fork 30k
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: remove multiple noop
definition
#47327
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
Define `noop` in `internal/util`. And use it elsewhere.
924a16a
to
95c28f8
Compare
Last time we checked, this was not a good idea: #38246 (comment) |
Thank you for feedback. I didn't know that using primordials may cause performance regression. |
Should there be a noop at all? Engines are pretty good at optimizing an empty inline function, last i was aware. |
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.
I'm -1 with this change. I suspect noop in multiple files is going to be less efficient due to be closing on a different module. There is also no line change benefit, because replacing a one liner with another is not really an advancement.
FWIW I see no performance regressions when running the HTTP benchmarks.
|
What if all noop functions are replaced with actual empty arrow functions? |
Define
noop
ininternal/util
. And use it elsewhere.