Simpler and faster wrapping #49
Conversation
|
Do we have any performance evidence here? |
|
With main as it is right now, after merging #46 , going through an array of 200k items with |
|
@jeswr this PR now includes your work from #55 , as discussed on Gitter. @jeswr @RubenVerborgh I've tried reworking the |
|
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. |
|
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 ! |
|
@jeswr I should have addressed both points in my latest commit. The names are a bit confusing, though, particularly the |
|
@jacoscaz , all of the |
|
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. |
| map<T>(map: (item: D) => T, self?: any): AsyncIterator<T> { | ||
| return new MultiMapFilterTransformIterator(this._source, { | ||
| filter: false, | ||
| function: self ? map.bind(self) : map, | ||
| next: { | ||
| filter: false, | ||
| function: this._map, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| filter(filter: (item: D) => boolean, self?: any): AsyncIterator<D> { | ||
| return new MultiMapFilterTransformIterator(this._source, { | ||
| filter: true, | ||
| function: self ? filter.bind(self) : filter, | ||
| next: { | ||
| filter: false, | ||
| function: this._map, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Per this comment it still may be worth removing these lines. @jacoscaz it might be worth doing some perf. tests on your machine to see if your results align with mine.
There was a problem hiding this comment.
I think the cause of the marginal slowdown observed there was the fact that we are essentially adding an extra function for every map operation in order to compose them (item: any) => t(_func(item));
There was a problem hiding this comment.
Curiously enough, I am getting 10% - 15% faster iteration by keeping those lines in. Removing them seems to slow things down. On some level this makes sense as V8 should optimize execution of the "compiled" transformation more effectively. Another differentiating factor might be the number of item vs. the number of iterators. These lines likely produce higher setup latencies, so they might have a more significant impact on scenarios with low numbers of items and high numbers of iterators. I would keep these in and postpone further tuning to a later stage as we're currently lacking dedicated scaffolding for this sort of testing.
…end events on promisified sources
…atorLike, adds option to prioritize ES2015 Iterable and Iterator while wrapping
…m ES2015 Iterator
|
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:
|
|
Superseded by #57 |
4 participants
This PR aims to extend the gains found in #48 to the
wrapfunction, skipping any kind of buffering when possible (i.e. when no transform options are provided). @jeswr suggested this in #44 .