Performance of for await of (async iteration) #31979
Comments
|
I'm sure there's some clever name for this like 'async iteration performance impacted by transform stream event amplification' ;) |
|
@alubbe are you saying that it calls emit('data') synchronously? If yes, nothing using promises will ever be faster. That being said, can you point to where the async iterator implementation is calling setImmediate? I can't find such a thing. |
devsnek
added
performance
|
@nodejs/streams |
|
Note that you are adding an additional I'm not sure it's all due to that, but that would be my first starting point for improving the perf there. From my tests which consisted in simply doing some reading from a file, the async iterators had similar performance, but there might be something that I did overlook. Can you produce a couple of self-runnable example that just use node core? We'll look into them if there are some ways in which we can optimize things. |
|
The Thanks for all of your input so far, I will try to build an example around readline as soon as I can. |
|
And here it is: https://github.com/alubbe/readline-benchmark The repo benchmarks 3 ways of using readline. Firstly, using the stream interface: await runProfiling('readline stream interface', () => new Promise((resolve, reject) => {
const rl = readline.createInterface({
input: fs.createReadStream('big.txt'),
});
let i = 0;
rl.on('line', (line) => {
i += 1;
});
rl.on('error', reject);
rl.on('close', () => {
console.log(`Read ${i} lines`);
resolve();
});On my mac (node 13.9.0), this takes between 32 and 35 ms and uses around 3 MB of memory. Then, it benchmarks async iteration as recommended in the docs: await runProfiling('readline async iteration', async () => {
const rl = readline.createInterface({
input: fs.createReadStream('big.txt'),
});
let i = 0;
for await (const line of rl) {
i += 1;
}
console.log(`Read ${i} lines`);
});On my mac (node 13.9.0), this takes around 50 ms and uses around 9 - 10 MB of memory. Lastly, I modified two lines in the prototype of readline's await runProfiling('readline async iteration via array of lines', async () => {
const rl = readline.createInterface({
input: fs.createReadStream('big.txt'),
});
let i = 0;
for await (const lines of rl) {
for (const line of lines) {
i += 1;
}
}
console.log(`Read ${i} lines`);
});On my mac (node 13.9.0), this takes between 30 and 34 ms and uses around 3 MB of memory. Here's the full output: |
|
This reproduces my excel.js issue quite neatly - the event amplification causes a performance dip, both in cpu and memory. Is there anything library authors, node core or v8 can do or should we recommend this for await of + for of combination for performance sensitive/low-level libraries? I remember reading that the async/await spec allows for optimizations around Promises around sync returns or Promises that have already resolved, but I have no idea if that could apply here. |
|
readline asynciteration has known performance issues (as it adds an internal transform stream) and therefore is not really representative as we would focus in removing that Transform instead. |
|
yes, but benchmark 3 uses that internal transform and is as fast as benchmark 1, which does not, right? If you'd like to try this out with something else, let me know what I should benchmark. Also, benchmarks 2 and 3 both use the internal transform but there's a big performance difference, so I think it is an independent discussion to removing the transform (which of course is a great idea!) |
|
I could also write a naive readline implementation as a transform stream from scratch (just to focus on the amplification of events) and benchmark that - would that help you since it wouldn't use code that is not yet fully optimized? |
|
That would help, yes. |
|
(Btw did you mean this extra readable stream here when you said transform Lines 1073 to 1104 in efec681 I've created two naive readline implementations as transform streams: class NaiveReadline extends Stream.Transform {
constructor() {
super({readableObjectMode: true, writableObjectMode: true});
this.buffer = '';
}
_transform(chunk, _encoding, callback) {
let pausing = false;
chunk = this.buffer + chunk;
const lines = chunk.split(lineEnding);
this.buffer = lines.pop();
for (const line of lines) {
if (!this.push(line) && !pausing) {
pausing = true;
this.pause();
}
}
if(!pausing) return callback();
this.once('readable', callback);
}
};
class NaiveReadline2 extends Stream.Transform {
constructor() {
super({readableObjectMode: true, writableObjectMode: true});
this.buffer = '';
}
_transform(chunk, _encoding, callback) {
chunk = this.buffer + chunk;
const lines = chunk.split(lineEnding);
callback(null, lines);
}
};I hope they are correct enough for this benchmark (obviously a ton of error handling and event listener clean up is missing). In any case, let's consume it via stream events: await runProfiling('naive readline stream interface', () => new Promise((resolve, reject) => {
const rl = new NaiveReadline();
let i = 0;
rl.on('data', (line) => {
i += 1;
});
rl.on('error', reject);
rl.on('end', () => {
console.log(`Read ${i} lines`);
resolve();
});
fs.createReadStream('big.txt').pipe(rl);
}));On my mac (node 13.9.0), this takes between 32 and 35 ms and uses around 3 MB of memory (same as node's readline). Let's benchmark async iteration: await runProfiling('naive readline async iteration', async () => {
let i = 0;
for await (const line of fs.createReadStream('big.txt').pipe(new NaiveReadline())) {
i += 1;
}
console.log(`Read ${i} lines`);
});On my mac (node 13.9.0), this takes around 50 ms and uses around 4 - 10 MB of memory. Again, similar to node's readline. Lastly, let's test NaiveReadline2 with its simple 1 event -> 1 array of events setup: await runProfiling('naive readline2 async iteration via array of lines', async () => {
let i = 0;
for await (const lines of fs.createReadStream('big.txt').pipe(new NaiveReadline2())) {
for (const line of lines) {
i += 1;
}
}
console.log(`Read ${i} lines`);
});On my mac (node 13.9.0), this takes around 30ms and uses around 3 MB of memory - again highlighting the original issue of event amplification slowing down for await of. Here's the full output: Let me know if this is more helpful or if there's any other checks you'd like me to run. |
Yes. Some notes:
One thing you might want to try is to ditch the Transform stream completely and async iterate the |
|
Regarding 2., I actually first wrote a version with Regarding your suggestion, could you write some pseudo code of what you have in mind? Are you thinking of an async iterator that yields each produced line vs. one that yields an array of lines? |
|
Something like: for await (let chunk of fs.createReadStream(file) {
const lines = ...
for (let line of lines) {
yield line
}
} |
|
I've finally found some time to revisit this - and in short, the above is indeed a lot slower than yielding the Here is what I benchmarked (all files are in the repo): const fs = require('fs');
const NaiveReadline = require('./NaiveReadline');
const NaiveReadline2 = require('./NaiveReadline2');
const runProfiling = require('./runProfiling');
const lineEnding = /\r?\n|\r(?!\n)/;
(async () => {
await runProfiling('naive readline stream interface', () => new Promise((resolve, reject) => {
const rl = new NaiveReadline();
let i = 0;
rl.on('data', (line) => {
i += 1;
});
rl.on('error', reject);
rl.on('end', () => {
console.log(`Read ${i} lines`);
resolve();
});
fs.createReadStream('big.txt').pipe(rl);
}));
await runProfiling('naive readline async iteration', async () => {
async function* readline(stream) {
let buffer = '';
for await (let chunk of stream) {
chunk = buffer + chunk;
const lines = chunk.split(lineEnding);
buffer = lines.pop();
for (const line of lines) {
yield line
}
}
}
let i = 0;
for await (const line of readline(fs.createReadStream('big.txt'))) {
i += 1;
}
console.log(`Read ${i} lines`);
});
await runProfiling('naive readline2 async iteration via array of lines', async () => {
async function* readline2(stream) {
let buffer = '';
for await (let chunk of stream) {
chunk = buffer + chunk;
const lines = chunk.split(lineEnding);
buffer = lines.pop();
yield lines;
}
}
let i = 0;
for await (const lines of readline2(fs.createReadStream('big.txt'))) {
for (const line of lines) {
i += 1;
}
}
console.log(`Read ${i} lines`);
});
await runProfiling('naive readline3 async iteration via an iterator', async () => {
async function readline3(stream, iterator) {
let buffer = '';
for await (let chunk of stream) {
chunk = buffer + chunk;
const lines = chunk.split(lineEnding);
buffer = lines.pop();
for (const line of lines) {
iterator(line)
}
}
}
let i = 0;
await readline3(fs.createReadStream('big.txt'), line => {
i += 1;
})
console.log(`Read ${i} lines`);
});
})();And here are the benchmarking results: |
|
Have you tried to use Overall you are correct: |
|
Unfortunately, Going back to the original question of this issue, is this a fundamental thing or is it something where the spec allows a type of micro-tick optimization that v8 could implement, similar to https://v8.dev/blog/fast-async ? |
|
The vast majority of the time is spent inside V8 and it is due to the overhead of As you can see in https://upload.clinicjs.org/public/6e4f252b53ea9834559fff3bc24d62283900d7a565cb99ef9bdae1c459deaf15/28374.clinic-flame.html#selectedNode=50&zoomedNode=&exclude=8000&merged=true, a signficant amount of time is spent in managing the generators. Here is a flamegraph done with linux perf: As you can see most of the overhead is in the the logic V8 uses to handle async generators :/. cc @nodejs/v8, do you think there is any optimization possible? |
|
|
|
yeah, // 1 - every chunk gets yielded individually, easy to use, currently a lot slower than 2
for await (const chunk of iterable) { ... }
// 2 - chunks get yielded as arrays as they are produced, forces the consumer to write a second for loop, but much faster
for await (const chunks of iterable) {
for (const chunk of chunks) { ... }
} |
|
I had a couple of more thoughts this morning:
export async function* transformSomeStreamGenerator(stream, opts) {...}
export async function transfromSomeStream(stream, opts) {
return Readable.from((function*() {
for await (const chunks of transformSomeStreamGenerator(stream, opts)) {
for (const chunk of chunks) {
yield chunk
}
}
})())
}This way Any thoughts on this? |
|
You are missing a few In the 2) case, I can't see if it will have any perf benefit at all. |
Added ;)
It would allow two ways of using it: // 1 - every chunk gets yielded individually, easy to use
// currently a lot slower than 2
for await (const chunk of transfromSomeStream(stream)) { ... }
// 2 - chunks get yielded as arrays as they are produced,
// forces the consumer to write a second for loop, but much faster
for await (const chunks of transformSomeStreamGenerator(stream)) {
for (const chunk of chunks) { ... }
}Happy to write another benchmark for this - but again, this would assume there is no way for v8 to achieve something close to parity. Can anyone comment on that? |
|
I don't understand why you are using |
|
Sure, let me give you some context. I'm re-writing the internals of exceljs, throwing out streams and replacing them with generators (sync wherever possible, async when needed). However, for backwards compatibility, we still need to expose a stream-like, eventemitter-based interface to our consumers. So, our main logic is now implemented in a export async function read(stream, opts) {
return Readable.from((function*() {
for await (const chunks of parse(stream, opts)) {
for (const chunk of chunks) {
yield chunk
}
}
})())
}Now, my worry is that the consumers will start using for-await on To re-state my question, is there any way for v8 optimizations to achieve parity or something close to it? I.e., can we somehow avoid paying the full price of constantly going back to the microtask/event queue on each iteration, when we already have the next element and could just do the iteration immediately? |
|
es.discourse.group is an official tc39 forum full of delegates. if someone there is interested I'm sure they will offer. |
|
I'll take that as a no. Ah well, thanks for the fast response. |
|
I can't be your champion, but your cheerleader ;) What you're proposing is exactly what I was after, because in my investigations here and on the v8 issue it became clear to me that we're leaving performance on the table, but we'd probably need to fix/improve this at the language level. I lack the knowledge to write this up, but I'm going to follow the discussions on https://es.discourse.group/t/syncandasynciterator/554 Thank you for taking this up! |
|
if though it's hopeless, I think it's a problem on es layer, not node layer... |
|
That's essentially my line of thinking, and I agree that the problem is on the ES layer. I don't think you can change the semantics of the existing for-await syntax through because it explicitly turns sync iterators into async iterators. The semantics you suggest would be a breaking change in that situation. If you change them to explicitly exclude sync iterators in order to avoid the breaking change, then the API feels inconsistent to me. That is still a variant that could be considered, if anyone seemed to be considering the proposal. I'm hoping to create more demand for a solution soon. |
|
Hello, @alubbe! I don't know if this can be of any help, but I was making some tests with your benchmark trying to improve the for await performance somehow, and I came up with this code (still not faster than just listening to the events): function getAsyncIterable(rl) {
return {
[Symbol.asyncIterator]() {
let onError;
let onClose;
let onLine;
let queue = {};
let error;
onError = (value) => {
rl.off('close', onClose);
rl.off('line', onLine);
error = value;
};
onClose = () => {
rl.off('error', onError);
rl.off('line', onLine);
queue = undefined;
};
onLine = (value) => {
if (queue) {
const node = { value };
if (queue.last) {
queue.last = queue.last.next = node;
} else {
queue.last = queue.next = node;
}
}
};
rl.on('line', onLine);
rl.once('error', onError);
rl.once('close', onClose);
function next() {
if (!queue) {
return { done: true };
}
if (error) {
throw error;
}
if (queue.next) {
const { value } = queue.next;
queue.next = queue.next.next;
if (!queue.next) {
queue.last = undefined;
}
return {
value
};
} else {
// If there's no element on the queue, I deactivate the queue filling and will just create a promise
// That resolves with the next line event
rl.off('line', onLine);
return new Promise((resolve, reject) => {
let onErrorOnce;
let onCloseOnce;
let onLineOnce;
onErrorOnce = (value) => {
rl.off('close', onCloseOnce);
rl.off('line', onLineOnce);
reject(value);
};
onCloseOnce = () => {
rl.off('error', onErrorOnce);
rl.off('line', onLineOnce);
resolve({ done: true });
};
onLineOnce = (value) => {
rl.off('close', onCloseOnce);
rl.off('error', onErrorOnce);
// Just before returning the listened value, I re-enable the queue processing
// so no message is lost
rl.on('line', onLine);
resolve({ value });
};
rl.once('line', onLineOnce);
rl.once('error', onErrorOnce);
rl.once('close', onCloseOnce);
});
}
}
return {
next
};
}
};
}
await runProfiling('readline manual readline async iterable', async () => {
const rl = readline.createInterface({
input: fs.createReadStream('big.txt'),
});
const iterable = getAsyncIterable(rl)
let i = 0;
for await (const line of iterable) {
i += 1;
}
console.log(`Read ${i} lines`);
});This actually creates an iterable based on event listening, which was an idea the I took from here. With this code, I could get an iteration with duration of 49ms, opposing to the for await over rl, which took 78ms, but still slower than the event listening, 38ms. |
|
@Farenheith That's interesting, but as far as I know it's not really the problem being discussed here. This thread is about the perf cost of the for await of loop itself, which is a very minor part of the cost of executing your benchmark. Converting from a stream to an iterable has moderate cost, but it need only be incurred once. Basically you are testing This is because the functional style often involves a chain of small incremental modifications, at which point the controlling factor for speed is the actual cost of |
|
Well sorry it is being discussed. I'm just not thinking about that as the most major problem, but you weren't talking to me anyway! |
|
@conartist6 hello! Well, yes, mate. It was discussed, thank you for noticing it. I agree that it's not the most important issue, as the overhead that multiples for-await-of put can affect a lot of other cases. But in this specific issue that @alubbe pointed out, this is relevant. I think, maybe, there's also a problem with the readline's Symbol.asyncIterator implementation, as with the code above I could get a performance 37% better than with the vanilla one. About the proposal you opened, @conartist6 I really hope this someday got implemented! We have heavy use of async iterables in the place I work and I also notice some time ago that we could benefit if the for-await-of could run the code synchronously when it receives a sync result from next. I created even a package to have that functionality, and I just uploaded it with a benchmark test based on @alubbe code and some code improvements. You can see the results here if you want: https://github.com/Codibre/augmentative-iterable/runs/4499575249?check_suite_focus=true You can notice that, while the iteration over a vanilla readline async iterator takes 97ms (readline async iteration profiling started), iterating over the Symbol.asyncIterator implementation I put above took 59ms (readline manual readline async iterable profiling started), iterating using the package, though, took 48ms (readline augmentative-iterable profiling started). As I implemented the logic in JavaScript and already got some performance improvement, if your proposal is implemented directly in the V8, then this could be even better! But again, I think there are some improvements that could be done in the readline's Symbol.asyncIterator implementation, otherwise, I couldn't get a performance boost with the code above (or maybe there's some bug on my code) |
|
I'm still sort of working on tackling this. My plan is to implement iter-tools with generators but then create custom transpilation to compile the generators down to classes. The classes would support the standard All I have to do then is ensure that my own transforms prefer the |
|
Actually now that I look more carefully at your code, that's basically what it's doing. I can't interpret the benchmark results though. I'd be very curious to see how it fares in the benchmark in |
If there are performance optimizations to be done in the readline module, please open a PR. I don't think the current implementation was benchmarked in any way. Please tag me on them, I'll be happy to provide guidance/review. |
|
@mcollina thank you for suggesting it! I'll be happy to do that. I'll try it this weekend |
|
Hello, @mcollina! I've been studying the issue and created even a package to test the concept I had in mind: https://github.com/Farenheith/faster-readline-iterator. It performed very well, in some cases it's 50% faster than the original async iterator, but I don't think I can implement something like that on NodeJs without introducing a breaking change, so I'm giving up lol. Some tests have broken because of that: I think I got the backpressure part covered so I could even rewrite that test, but the lack of the stream property can cause unexpected behaviors. I also uploaded a version of @alubbe benchmark with two new cases:
|
I think a minimal breaking change (dropping |
|
I just wanted to say that it's awesome that we have some many performance hungry people in the community that almost 2 years after this issue was raised we're still making progress here - so thanks to all of you here and happy holidays! |
It's likely that Deno has the same problem as the following issue that exists in Node. nodejs/node#31979 So we tried to avoid this problem by using `AsyncGenerator<T[]>` instead.
|
Tangently related, I have a verbose breakdown of the differences and reasoning here: https://stackoverflow.com/a/70979225/1426683 I find the |
|
So I've been tinkering with the design of coroutine-based async/streaming parsers for the last while now. When I proposed these changes I was unable to find a champion, but I don't think I had yet made the strongest possible case for how important this functionality is. I've now designed and built a streaming parser framework based on co-recursion. I think there will be great demand for this implementation, but I also think that the people adopting the technology will quickly face this problem. For the moment I know how to deliver a hacked-up fix, but I still really think the proposal to amend the language has strong merit and I would loooove to find someone willing to take up the process at all. |
|
Could you share some details on what you implemented, what the performance differences still are to using streams and events, and what the hack is that you're referring to? |
|
To start with, the hack is implementing semi-sync iterables without support from the language. I now control a large enough ecosystem that if my tools support those kinds of streams they will have an ample arrays of sources and sinks, making them usable in a practical sense. But of course if there's no standard, I'm likely to create a mess because what I'll be creating is a "transitional" way to use semi-sync iterables something like In terms of the difference to streams and events, the most major difference is that data flow is in the complete opposite direction. Instead of data driving callbacks, data processing drives consumption of data. I consider this to be "uninverted" control flow because this is the way that we have all the facilities of structured exception handling. With inverted flow exceptions bubble up to the async callback that received a chunk of the file from disc, and that handler likely has very little context about the application! Another win that derives from keeping control flow right-side-up is that we gain extensibility. Because the caller is consuming productions from the input stream (based on current state if necessary), you can extend a language using facilities that are remarkably similar to class extension. |
Async iterable is slower than sync iterable because.........async is slower than sync. Iteration is pretty much irrelevant, other than iteration implies it happens lots of times. I think chunks of data (and iterating synchronously over each chunk) is the right approach, or at least one that I've accepted.
You can design everything around generators. You set up a generator framework (not shown) that lets you write stuff like: function *parseLines(source, dest) {
let buffer = "";
while (true) {
const next = yield source.read();
if (next === undefined) {
break;
}
const parts = (buffer + next).split("\n");
for (const part of parts.slice(-1)) {
yield dest.write(part);
}
buffer = parts[parts.length - 1];
}
if (buffer) {
yield dest.write();
}
}Where |
I hope this is the right place to ask the question and that this hasn't already been discussed to death elsewhere. Feel free to direct me elsewhere or close the issue if I've missed something.
In short, I am one of the maintainers of exceljs (which is basically a transfrom stream taking in a read stream, unzipping its contents, running an xml parser on the unzipped chunks and then emitting back excel-related events) and we're in the process of adding support for async iteration via for await of (exceljs/exceljs#1135).
In doing that, we've noticed that for await of is significantly slower than the current
.on('data',..)based approach. Our benchmark is not a microbenchmark, but a full end-to-end benchmark incl. creating and analyzing excel objects in memory (exceljs/exceljs#1139). Switching to for await of (vs. handling the events in sync callbacks) decreased performance by around 60%.I have debugged this issue (lddubeau/saxes#32) and in short, the issue arises because for every chunk/event passed into our transform stream, we emit out a magnitude greater of chunks/events. And so what's causing the performance is that the callback code would run through these emitted chunks/events mostly synchronously, whereas the current implementation of
Symbol.asyncIteratoronReadablecallssetImmediatebetween each event, which is quite expensive. I wrote a simple microbenchmark to compare for of against for await of on the same array or iterator, and the difference is around 10x.So we've come up with this 'hack' where instead of emitting one-by-one all of these chunks/events that our transform produces, we now gather them up in an array and emit that once. Or phrased another way, instead of calling
this.push()for every excel related event that we produce, we call, for each chunk written into our stream, a lot ofthis.events.push()(wherethis.eventsis just an array that initialized in the constructor) and then finallythis.push(this.events)once we're done consuming the chunk (and we also resetthis.eventsto an empty array again). Clever, but now consuming the stream is ugly. Instead of writing `` we now writeI think this performance issue will bite a lot people because it's so easy to fall into and, at least to me, came as a surprise. I remember reading that
readlinehas similar performance issues (and similarly to the above it produces a lot more events than it takes in) and would probably also see performance improvements from the above approach.My question boils down to this: Is there a fundamental reason in the spec around async iteration or streams that we have to go to setImmediate if the read buffer still has stuff in it (i.e., if we could call
.next()synchronously? Is it something that v8 can/will eventually optimize? If no to both questions, what should library authors do to give users all the advantages of async iteration while not sacrificing performance?Roping in @BridgeAR as a fellow performance nerd and the only one I know here ;)
The text was updated successfully, but these errors were encountered: