Closed H4ad closed 11 months ago
In the past @aduh95 documented which primordials created a performance issue, I think we should add this there too.
It's in the primordials.md
document: https://github.com/nodejs/node/blob/HEAD/doc/contributing/primordials.md
Based on nodejs/node#49730, we can improve the performance of structuredClone
up to 50% (at least on my internal benchs) if we could remove ReflectConstruct
.
This could be beneficial for webstreams and blobs, but I was not able to make it work, I tried:
function InternalClonedBlob() {
markTransferMode(this, true, false);
}
ObjectSetPrototypeOf(InternalClonedBlob.prototype, Blob.prototype);
ObjectSetPrototypeOf(InternalClonedBlob, Blob);
InternalClonedBlob.prototype[kDeserialize] = () => {};
function ClonedBlob() {
var cloned = new InternalClonedBlob();
cloned.constructor = Blob;
return cloned;
}
Which throws:
node:internal/worker/js_transferable:38
throw new Error(`Unknown deserialize spec ${deserializeInfo}`);
^
Error: Unknown deserialize spec internal/blob:ClonedBlob
at node:internal/worker/js_transferable:38:13
at receiveMessageOnPort (node:internal/worker/io:410:19)
at structuredClone (node:internal/structured_clone:24:10)
at Object.<anonymous> (/home/h4ad/Projects/opensource/node-copy-3/test-khafra.js:3:13)
at Module._compile (node:internal/modules/cjs/loader:1429:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1487:10)
at Module.load (node:internal/modules/cjs/loader:1261:32)
at Module._load (node:internal/modules/cjs/loader:1077:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:99:12)
at node:internal/main/run_main_module:23:47
I was able to make a version that works doing:
class ClonedBlob extends Blob {
static {
markTransferMode(this, true, false);
this[kDeserialize] = () => {};
}
}
ClonedBlob.prototype.constructor = Blob;
It prints correctly the results of:
const { Blob } = require('buffer');
(async () => {
console.log(structuredClone(new Blob(['hello'])));
console.log(await structuredClone(new Blob(['hello'])).text());
console.log(structuredClone(new Blob([])).constructor);
console.log(structuredClone(new Blob([])).constructor === Blob);
console.log(new Blob(['a']).slice(0, 1).constructor);
})();
Blob { size: 5, type: '' }
hello
[class Blob]
true
[class Blob]
But when we run the benchmark, it throws an error after a couple executions:
[00:00:11|% 8| 0/1 files | 5/60 runs | 0/1 configs]: blob/clone.js node:internal/worker/io:410
const message = receiveMessageOnPort_(port?.[kHandle] ?? port);
^
Error: Unable to deserialize cloned data.
at receiveMessageOnPort (node:internal/worker/io:410:19)
at structuredClone (node:internal/structured_clone:24:10)
at main (/home/h4ad/Projects/opensource/node-copy-3/benchmark/blob/clone.js:17:20)
at /home/h4ad/Projects/opensource/node-copy-3/benchmark/common.js:54:9
at process.processTicksAndRejections (node:internal/process/task_queues:77:11)
Node.js v21.0.0-p
@rluvaton Do you have any hint?
On top of my head no but I will try to investigate it
I did not migrate transfer classes in webstream so did not encounter with this error
The error of unable to deserialize cloned data
could be a bug, I opened an issue https://github.com/nodejs/node/issues/49844.
I discovered when I was trying to optimize the histogram
, without changing the code, I got the error.
Legendecas opened a PR to fix the issue described in the previous issue, https://github.com/nodejs/node/pull/50026, I would appreciate it if anyone here could approve that PR, I'm already using it to remove usages of ReflectConstruct on transferable objects.
All PRs were merged, I think I already removed all the relevant references for ReflectConstruct, so I think we can close this as completed.
I think this is the second PR I saw removing the usage of
ReflectConstruct
: https://github.com/nodejs/node/pull/49546The first one improved a lot the creation of
WebStreams
, and there are much more cases where this function is used, so I think that it worth exploring the other usages to see if possible to remove that function.