Performance degradation with Node 16

[tomjw64]

Using the following minimal example:

const { mark, stop } = require('marky')
let iter = 0
while (++iter <= 20_000) { mark('test'); stop('test') }

leads to much slower performance is seen when using Node 16. Example run:

$ n
   installed : v16.13.2 (with npm 8.1.2)
$ time node index.js 
real	0m31.254s
user	0m30.745s
sys	0m0.882s
$ n
   installed : v14.16.1 (with npm 6.14.12)
$ time node index.js 
real	0m0.048s
user	0m0.042s
sys	0m0.017s

The reason for this degradation is that Node 16 performance.getEntriesByName() runs flatMap behind the scenes, an O(n) operation, where n is the number of marks in the buffer. In contrast, the marky implementation for Node 14 uses a simple O(1) object property access. This will heavily affect users that do not clear their marks on short intervals or leak them, intentionally or not.

[nolanlawson]

Thanks for reporting, I can reproduce. A 31s vs 0.048s difference is huge!

It looks like the main difference is that the global performance is defined in Node 16, but not in Node 14. So effectively, I fixed #17 without realizing it. 😅

I can think of two potential solutions:

1. I close Add support for Node.js Performance Timing API #17 as "never" and don't use the built-in Node performance API
2. We file a bug on Node for the performance issue with performance.getEntriesByName()

I can see a reasonable case for either one.

For instance, if we go with # 1, then we're assuming that Node's built-in API has nothing useful to offer. Whereas maybe in the future, it will offer something like a visualization of marks/measures in the Chrome debugger's Profiler (one can hope, right?).

Also if we go with # 1, now we permanently have a vast difference between the Node and browser version of marky. Effectively, we're treating Node as a polyfilled environment.

Personally I am inclined to go with # 2 since it seems sad to me to polyfill something that Node already offers out-of-the-box.

[nolanlawson]

On second thought, maybe it makes sense to switch away from Node's built-in API now, while filing a bug on them in parallel. There's no compelling reason (currently) to use the built-in versus using our own, and the slowdown is pretty massive.

[nolanlawson]

Fixed in v1.2.3. I haven't filed the bug on Node yet; if you get around to doing that, please let me know. 🙂

[tomjw64]

Submitted an issue against node: nodejs/node#42004

[tomjw64]

Would it also be an option to keep using the performance api, but to use the returned PerformanceMeasure object from the measure function instead of using getEntriesByName to find it?

[nolanlawson]

@tomjw64 Ah yes, this is an excellent point. I forgot that this was even an option. Opened an issue: #32

Thanks for filing the issue on Node!