Simpler and faster wrapping

[jacoscaz]

This PR aims to extend the gains found in #48 to the wrap function, skipping any kind of buffering when possible (i.e. when no transform options are provided). @jeswr suggested this in #44 .

[RubenVerborgh]

Do we have any performance evidence here?

[jacoscaz]

With main as it is right now, after merging https://github.com/RubenVerborgh/AsyncIterator/pull/46 , going through an array of 200k items with wrap(new ArrayIterator(arr)) takes ~103 ms (MacBook Pro, Apple Silicon, M1, 16 GB RAM). With this PR that goes down to ~10 ms, so roughly 10x faster.

[coveralls]

Coverage remained the same at 100.0% when pulling 10308913ee54c9ef6fec220dd5b7dcac80cd2653 on jacoscaz:faster-wrapping into 358608fa09ec87f90cd30190b8c0ffd96f727986 on RubenVerborgh:main.

[jacoscaz]

@jeswr this PR now includes your work from #55 , as discussed on Gitter.

@jeswr @RubenVerborgh I've tried reworking the wrap() function to return the most appropriate iterator for all sorts of sources while maintaining backward compat. with the current implementation. Seems to work well but looking forward to your feedbacks. This PR now builds on #48 , diff here https://github.com/jacoscaz/AsyncIterator/compare/faster-filtering-mapping...jacoscaz:faster-wrapping .

[jacoscaz]

I realize that the resulting PR might get pretty big but perhaps we could merge this one into #48 so as to have all of the recent performance work in one branch. That would simplify testing in other projects.

[jeswr]

I think the _wrap function still needs an option where you can select to run the isIterable tests first within the wrap function.

I'm also concerned that this is a little heavyweight if you do lots of wrapping, for instance within the map or transform of another iterator. I think it's worth having custom wrap functions for each of those source types that the user can choose to use if they know the nature of what they are wrapping in advance.

Great work on this btw @jacoscaz !

[jacoscaz]

@jeswr I should have addressed both points in my latest commit. The names are a bit confusing, though, particularly the fromIterator function (for ES2015 Iterator) and fromIteratorLike function (for AsyncIterator, stream.Readable, ...). Suggestions?

[jeswr]

@jacoscaz , all of the iteratorLike stuff could potentially be AsyncIteratorLike? Because the fromIterator is strictly for ES2015 synchronous iterables?

[jacoscaz]

All right, done! Let's see what @RubenVerborgh says; if things look good I'll add tests for both this and #48 and we should be all set.

[jacoscaz]

Happy to report that not only all tests are passing but with this branch we're not that far from having all tests in Comunica pass, too! There's only 8 tests that are breaking. Issues so far:

• There's one test suite that passes a set of initial quads as an empty object, which breaks wrapping https://github.com/comunica/comunica/blob/448bfbadd805dcad95841789cd12b0d33a672585/packages/actor-rdf-resolve-hypermedia-qpf/test/ActorRdfResolveHypermediaQpf-test.ts#L103 . Fixing this one is easy, just change the test to make that an empty array.
• There's one actor that uses the protected method BufferedIterator#_fillBuffer(), which is not available anymore as after these PRs there's no guarantee for wrap() to return an instance of BufferedIterator (and we shouldn't use protected methods anyway) https://github.com/comunica/comunica/blob/448bfbadd805dcad95841789cd12b0d33a672585/packages/actor-query-operation-sparql-endpoint/lib/ActorQueryOperationSparqlEndpoint.ts#L168 . Fixing this one might not be as easy but I don't have a clear idea of what's going on.
• This test https://github.com/comunica/comunica/blob/448bfbadd805dcad95841789cd12b0d33a672585/packages/actor-query-operation-sparql-endpoint/test/ActorQueryOperationSparqlEndpoint-test.ts#L410 is breaking due to a timeout while expecting an error event. This might be due to something missing in how we relay errors within these PRs.

[jacoscaz]

Superseded by #57