iter-ops
iter-ops copied to clipboard
vitaly-t
Removing async/await code
When I started on this library, a lot of time asynchronous implementation was simply copied from synchronous, with added async/await usage, in order to save time (plus some operators have fairly complex logic).
However, performance tests showed that native .then.catch solution performs way better than async/await, so I've been converting async operators to use .then.catch syntax only (see #76).
There are still some operators remain that rely on async/await syntax. Mostly it is because they have fairly complex logic, which makes conversion quite a challenge.
This issue is to track the async operators that still need to be converted:
-
concat- this should take priority, as the most frequently used one -
defaultEmpty -
repeat -
split- the most complex operator in the library, likely with some issues - #33
P.S. @RebeccaStevens In case nothing else challenges you here :wink:
Related: #76
I'm actually kind of curious whether using callbags would be the best approach. They're super performant and might even be able to dedup a lot of commonalities between the sync and async code.
I do not see how callbacks can be used there, since every operator is expected to return a Promise.
It would take a bit of research to see if that would even be possible to use, given this library's objectives and current function. And if it were, this issue here is about the implementation aspect, strictly. I prefer to keep it simple.
I've done a bit of testing on whether using callbags would be a viable approach.
Here's how a couple of the operations would look:
export const aggregate =
<A, R>(cb: (a: A[]) => R) =>
async (self: Source<A, unknown>): Promise<R> =>
cb(await asyncIterableToArray(toAsyncIterable(self)));
export const count =
<A>(cb: (value: A, index: number) => Promise<number> | number) =>
async (self: Source<A, unknown>): Promise<number> => {
let count = 0;
let index = 0;
for await (const element of toAsyncIterable(self)) {
const result = await cb(element, index++);
if (result) {
count++;
}
}
return count;
};
Of course, async/await can be removed from this code but it won't be as elegant. Those implementations above can be used from either a sync or async starting point. A slight performance increase for sync can be achieved with a dedicated sync version of these operations:
export const aggregateSync =
<A, R>(cb: (a: A[]) => R) =>
(self: Source<A, unknown>): R =>
cb([...toIterable(self)]);
export const countSync =
<A>(cb: (value: A, index: number) => boolean) =>
(self: Source<A, unknown>): number => {
let count = 0;
let index = 0;
for (const element of toIterable(self)) {
const result = cb(element, index++);
if (result) {
count++;
}
}
return count;
};
Callbags will also allow for more power pipelines.
For example, here pipe returns a Promise<number>.
const output = await pipe(
fromIter([1, 2, 3]),
aggregate((arr) => {
return arr.reduce((a, c) => a + c);
})
);
expect(output).to.eql(6);
And with the explicit sync operator, pipe returns a number.
const output = pipe(
fromIter([1, 2, 3]),
aggregateSync((arr) => {
return arr.reduce((a, c) => a + c);
})
);
expect(output).to.eql(6);
I believe using callbags would be quite beneficial but would take a bit of work to implement (and will of course be a breaking change).
Let me know if you want me to continue to pursue this.
Thank you for looking into this!
The priority here is to get rid of the async/await syntax, because it slows things down quite a bit, or at least according to some tests I did in the past.
However, the whole issue isn't really a big deal, certainly not one to justify another major change. That's why I'd prefer to leave it without making any such big change, at least presently.
I appreciate all the work you've been putting into this! And since you like good challenges, then issue #33 is certainly worth looking into. It is the most complex operator that we have, and I believe some function there is off, along with the tests for it.
P.S. Oddly enough, my WebStorm UI fails to detect iterable type correctly when we have more than 2 concat parameters, as shown below... even though our own test seems to pass there.
