Fast path for --no-deprecation

[H4ad]

About the fast path

In the code of process.emitWarning, deprecations are ignored when --no-deprecation flag is passed (reference).

Instead of creating the warning object and then discarding when the flag is true, we could just check earlier and just skip the rest of the method when type is DeprecationWarning.

There are some places that could benefit from this tiny change.

Going further with improvements

What if we add guards to those places to avoid any computation at all when --no-deprecation is true?

I did some experiments with esm internal resolve and by putting an if guard for process.noDeprecation on line 106, and the results is not that bad:

                                                                              confidence improvement accuracy (*)   (**)   (***)
esm/cjs-parse.js n=100                                                                       -0.69 %       ±2.91% ±3.91%  ±5.16%
esm/esm-loader-defaultResolve.js specifier='./relative-existing.js' n=1000                    0.76 %       ±4.52% ±6.12%  ±8.20%
esm/esm-loader-defaultResolve.js specifier='./relative-nonexistent.js' n=1000                 0.26 %       ±4.80% ±6.47%  ±8.62%
esm/esm-loader-defaultResolve.js specifier='node:os' n=1000                            *      7.18 %       ±6.08% ±8.14% ±10.71%
esm/esm-loader-defaultResolve.js specifier='node:prefixed-nonexistent' n=1000                -2.21 %       ±6.18% ±8.30% ±10.96%
esm/esm-loader-defaultResolve.js specifier='unprefixed-existing' n=1000                *      9.70 %       ±7.34% ±9.84% ±12.96%
esm/esm-loader-defaultResolve.js specifier='unprefixed-nonexistent' n=1000                    1.14 %       ±2.07% ±2.77%  ±3.64%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 7 comparisons, you can thus
expect the following amount of false-positive results:
  0.35 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.07 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

node benchmark/run.js esm

I think the results could be even better, this benchmark is a little bit flaky as you can see by the accuracy.

What about --no-warnings?

Well, at first I thought we could do the same for --no-warnings, add an if statement and avoid calling process.emitWarning, but NodeJS just unsubscribe the listener when --no-warnings is set , but they won't stop the event from being emitted, also, there are MANY places where process.on('warning is used, so I don't think we could do anything here.

Conclusion

So, in the end, we have two questions:

• Does it worth moving the noDeprecation before the object creation?
• Does it worth adding fast path for every location that emits deprecations?

[Ceres6]

Hi, I'll be working on this. I will be posting a draft PR with my progress

[Ceres6]

@H4ad about the no-warnings point. Couldn't we if (process.noProcessWarnings) return; at the beginning of the emitWarning function? As far as I can see few codebases actually use process.emit('warning') and lots of them use the process.emitWarning so it should be useful to avoid creating the events there.

[H4ad]

@Ceres6 I initially thought the same, but --no-warnings behaves differently than --no-deprecation.

You can still listen for events emitted by --no-warnings, they just won't be printed to the console.

With --no-deprecation they stop emitting these events.

So stopping emitting will likely be a breaking change.

What I recommend is to create a PR related to --no-deprecation and then create another one related to --no-warnings, I don't know who would decide on the behavior of --no-warnings, so creating a PR will be faster to validate this change.

Also, try researching whether anyone has already proposed changing the behavior of --no-warnings, which might be useful to avoid working in an idea that was previously rejected.

[Ceres6]

@H4ad As far as I can see there are no other issues proposing the change. It is weird as per documentation both flags should have the same behaviour (i.e. silence warnings). If that behaviour is not printing to console or not emitting could be discussed.

I think this should at least be documented if they are going to have a different behaviour.

Anyway I do agree it is probably for the best to keep that in separate PRs.

I have a couple options in mind so I will try them with existing benchmarks and then maybe create some. Probably it's worth to compare also without the flag to check we are not sacrificing performance there in order to gain for the other (although I suspect a couple if statements won't affect too much)

[aduh95]

I'm not sure it's relevant to spend any time optimising deprecated features 🤔 if folks want performance, they should use not-deprecated APIs, I don't really get who'd be benefitting from an optimised --no-deprecations.

[H4ad]

@aduh95 In parts I agree with you, but I see this problem as an entry level to do optimizations in node, you don't need to change C++ code or have a deep understanding of node to be able to help.

And as for people benefiting directly, if this change improves module loading or any other part by a few percentages, then adding a new flag might be worth someone's effort.

[Ceres6]

@aduh95 as I see it that you are not optimizing for performance doesn't mean that you want a system as performant as possible.

Besides there are a number of reasons for using deprecated APIs such as having a legacy system with thousands of calls to those APIs that you cannot invest the time needed to refactor

[mmarchini]

having a legacy system with thousands of calls to those APIs that you cannot invest the time needed to refactor

Maybe some pre-work before investing time in optimizing this code path would be to implement trace events or perf hooks for deprecation warning calls. Or even just taking a flamegraph of production systems to identify if those are incurring any significant overhead. That way, improving the code path for --no-deprecation would be a decision based on concrete data that this is actually helpful for those kind of systems.

Having that data, especially in a production system, would also be helpful to advocate for landing such changes whenever someone opens a PR for it. Otherwise, it might be a hard sell if the changes incur significant increase in complexity code for insignificant performance gains in real systems.