-
Notifications
You must be signed in to change notification settings - Fork 7
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
Blob.text and Blob.arraybuffer are 10x+ slower on 20.x #118
Comments
Some benchmark data: NodeJS 16.20.1
NodeJS 18.17.0
NodeJS 20.5.0
|
@H4ad run a git bisect to find where the regression is? |
@benjamingr I will! Also, this regression was so hard to run this piece of code: const buff = Buffer.allocUnsafe(1024 ** 2);
const source = new File(buff, 'dummy.txt', options);
const now = performance.now();
source.text().then(() => console.log(`diff: ${performance.now() - now}`)); It took 3s on node 20.x and 80ms on 18.x. |
Which public api affected by this? |
If I understand your question, From what I read, |
This will be a tricky one as the new mechanism underlying the C++ implementation of Blob is also going to be used for the QUIC implementation that I'm currently working on. It also serves as the underlying bit for file-backed |
In the QUIC implementation, are you changing the Blob? |
Making an initial attempt here nodejs/node#49873 just stuck on how to make the pulling part async similar to how we are doing |
so I kinda did git bisect and I suspect it was either created by: or created by those: |
Hmm the main problem seems like we pass callback -> callback again passes back value after reading -> repeat 2 JS -> C++ and back calls everytime data being read |
Pretty sure you were looking for |
ahah yes! would do the job i guess but bigger problem rn is how to queue them described in nodejs/node#49873 (comment) |
@jasnell how is going the QUIC Implementation? Do you think we can go back and look for ways to optimize or do you think we should wait until we have more updates on the QUIC implementation? |
Made some progress on this in nodejs/node#49873 but stuck on some C++ pointer stuff if anyone could help or take a look would be great |
The
blob/blob.js
benchmark is taking hours: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1402/consoleThis probably didn't take so long in the past, there is this PR that changed the Blob's behavior a lot: nodejs/node#45258
From what I saw, the problem is in the JS->C++ transitions, for each piece of data it crosses JS->C++.
Furthermore, the
text
function expectsarraybuffer
and then callsdecode
, when we could justdecode
the data as soon as we get it from the C++ instead of keeping everything in memory and then discarding it.I can do some optimizations on the JS side but I think this could be largely improved if we do the heavy operations on C++, I have no idea how to handle the asynchronous code on C++, so I accept some hints or if anyone wants to try work on this.
The text was updated successfully, but these errors were encountered: