-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
stream: replace queue and readRequests with buffer list for performance improvement #49554
stream: replace queue and readRequests with buffer list for performance improvement #49554
Conversation
8465d8b
to
4a982c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using FixedQueue would have better performance?
// TODO - may be able to change this to next next next | ||
while (reader[kState].readRequests.length > 0) { | ||
// TODO - why this is in the while loop? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove todos
Benchmark CI for https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1369/ |
5943045
to
4e39d86
Compare
the pipe-to benchmark takes too long, it's currently running for an hour. Each iteration (single combination) takes 9 seconds. there are 16 possible combinations. new Benchmark CI that has reduced the number of chunks from 5M to 100K for |
Each iteration (single combination) takes 9 seconds. there are 16 possible combinations. so 9 seconds * 16 combination * 30 runs * 2 versions = 2 hours and 24 minutes...
controller[kState].queueTotalSize = 0; | ||
} | ||
|
||
function peekQueueValue(controller) { | ||
assert(controller[kState].queue !== undefined); | ||
assert(controller[kState].queueTotalSize !== undefined); | ||
assert(controller[kState].queue.length); | ||
return controller[kState].queue[0].value; | ||
debugger; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
Could not make it work, the tests failed for some reason... current |
webstream possible improvement:
BufferList
implementationFrom my local tests:
Before
After