-
Notifications
You must be signed in to change notification settings - Fork 92
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
Typescript: provide message iterator which yields synchronously for messages #1188
Conversation
392be23
to
79ae3c2
Compare
2a92ec9
to
270a38a
Compare
|
||
/** Returns an iterator that iterates over messages in the MCAP file in order of log time. | ||
* The returned object will have either the `message` or `promise` member defined. If `promise` | ||
* is not undefined, the caller must wait for it to resolve before calling next(). |
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.
* is not undefined, the caller must wait for it to resolve before calling next(). | |
* is defined, the caller must wait for it to resolve before calling next(). |
The general takeaway here is that the "async" aspect of getting the next message is typically only async when it comes to loading the next chunk, and otherwise getting the next message is a "sync" operation where you can immediately grab the message from an internal data structure. My initial reaction to the proposed interface is lackluster - feels somewhat awkward but I'd be open to seeing this kind of interface in any prior art. Maybe another way to accomplish a similar result is to return an iterator that provides batches of messages? So your outer iterator get you some other iterable thing that will return the latest messages in the loaded batch until you need to "wait for more"? Its not obvious to me that this would be easier or more natural to use. |
Yeah - i agree the interface isn't elegant. I figure there's a place in the world for uglier-but-faster code.
As the caller I prefer one for loop with an if vs. two nested loops, but that's just my preference. |
I closed this before because I was going to pursue a nested-loop api, but i've decided against that now. My reason is that with this example API: async *readMessages(): AsyncGenerator<Generator<TypedMcapRecord["Message"], void, void>, void, void> It's too tempting for a caller to try to store the second-level generators between iterations of the top level. I can't think of a good design that both a) doesn't leak objects into orphaned closures and b) can handle iterators being stored gracefully. |
cc. @AaronO |
Changelog
Docs
Description
await
on every message significantly slows down iteration over messages, when there are many messages per chunk. See benchmark results here:That is roughly 30% faster.
This PR provides a new method (with name subject to change) that allows callers to only
await
when necessary.I'm not confident on the name for this method or return type, happy for feedback there.