`deprecate` shows increased use of CPU

[slagiewka]

Version

v24.0.0

Platform

Happening in many environments I've tested:
Docker: `Linux 1633ae2ed477 6.10.14-linuxkit #1 SMP Thu Mar 20 16:32:56 UTC 2025 aarch64 GNU/Linux`
Native macOS: `Darwin host.local 24.3.0 Darwin Kernel Version 24.3.0: Thu Jan  2 20:24:16 PST 2025; root:xnu-11215.81.4~3/RELEASE_ARM64_T6000 arm64`

Subsystem

No response

What steps will reproduce the bug?

A simple code with standard library only. Added some limiting to keep it running while inspecting. It might not be balanced for best performance and showing the major perf hit.

node --inspect index.js

for (let index = 0; index < 20_000; index++) {
  const proms = [];
  for (let index = 0; index < 100; index++) {
    proms.push(
      fetch("https://nodejs.org", {
        signal: AbortSignal.timeout(1000),
      })
        .then((res) => res.text())
        .then(console.log)
        .catch(console.log),
    );
  }

  await Promise.all(proms);
}

How often does it reproduce? Is there a required condition?

100% on node v24.

What is the expected behavior? Why is that the expected behavior?

No time spent on deprecate shows in performance tools.

What do you see instead?

Significant time spent on deprecate (real project shows higher use than the example).

Additional information

FWIW there are no deprecation warnings issued to the console output.

I do not see this behaviour in v23.

[slagiewka]

Decided to run undici benchmarks, I see worse performance in v24 across multiple runs. It might be indicative of some regression.

[slagiewka]

Seems like this is because #57865 is now in place.
However, I'm not sure if hitting this path should be so visible.

[juanarbol]

Hey, seems that you have a good picture of the performance issue. PRs are more than welcome. :)

[slagiewka]

Hey, seems that you have a good picture of the performance issue. PRs are more than welcome. :)

Currently I have not figured out what to do about it.

The problem is that there is bind happening on AsyncResource where the asyncResource prop has getters and setters deprecated. In order to deprecate these and allow for getting and setting actual values if required, the self / this is used. Therefore neither can be optimised to run only once since it depends on each instance it's binding to.

Unless deprecate is made faster (seems to spend most of the time on ObjectSetPrototypeOf -- obviously) or these getters and setters can be reused or avoided entirely, I don't know what to do with it. I'm not familiar with what's going on in the entire async hooks / context frame processes.

[bjohansebas]

cc: @nodejs/async_hooks

[slagiewka]

Seems like this is because #57865 is now in place.

With that now backported to v22, I see the same with v22.16.0 but not on v22.15.1.

[RafaelGSS]

This is now fixed on main. I'll investigate how we could make the deprecate faster when I get some free time

[RafaelGSS]

Right, so you are correct, the performance drawback was due to deprecate caused by #46432 and called by #57865. The main problem with our deprecate function is that it manipulates the prototype:

  // // The wrapper will keep the same prototype as fn to maintain prototype chain
  ObjectSetPrototypeOf(deprecated, fn);
  if (fn.prototype) {
    // Setting this (rather than using Object.setPrototype, as above) ensures
    // that calling the unwrapped constructor gives an instanceof the wrapped
    // constructor.
    deprecated.prototype = fn.prototype;
  }

  ObjectDefineProperty(deprecated, 'length', {
    __proto__: null,
    ...ObjectGetOwnPropertyDescriptor(fn, 'length'),
  });

I can confirm that removing these lines will significantly reduce the impact of deprecate. I believe that creating an alternative version to deprecate that doesn't change the prototype, but emits the DEPXXX warning, will solve this. I will work on something.

cc: @nodejs/undici @mcollina for awareness -- this reduced the fetch performance on Node.js 22 and Node.js 24 now.