Skip to content
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

WIP: V3 Sync Unions #65

Closed
wants to merge 5 commits into from
Closed

WIP: V3 Sync Unions #65

wants to merge 5 commits into from

Conversation

jeswr
Copy link
Collaborator

@jeswr jeswr commented Apr 10, 2022

Same idea as #54 but targeted for v3.x.

#63 and #59 should be merged first

Now also resolves #67

linkedlist.ts Outdated Show resolved Hide resolved
linkedlist.ts Show resolved Hide resolved
linkedlist.ts Show resolved Hide resolved
asynciterator.ts Outdated Show resolved Hide resolved
asynciterator.ts Outdated Show resolved Hide resolved
asynciterator.ts Outdated Show resolved Hide resolved
asynciterator.ts Outdated Show resolved Hide resolved
@jeswr
Copy link
Collaborator Author

jeswr commented Apr 15, 2022

Note: the failing test is because .map is buffering. This will be resolved once we rebase to #59

@jeswr jeswr requested a review from jacoscaz April 17, 2022 14:44
Copy link
Collaborator

@jacoscaz jacoscaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done, I like how the maxBufferSize is used to limit the number of source the iterator reads from. These are just minor nitpicks.

@@ -479,7 +479,10 @@ export class AsyncIterator<T> extends EventEmitter {
@returns {module:asynciterator.AsyncIterator} A new iterator that prepends items to this iterator
*/
prepend(items: T[] | AsyncIterator<T>): AsyncIterator<T> {
return this.transform({ prepend: items });
return new UnionIterator(
[Array.isArray(items) ? new ArrayIterator(items, { autoStart: false }) : items, this],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After #63 is merged, this could become simply wrap(items, { letIteratorThrough: true }) (thus supporting a wider range of types for items).

@@ -489,7 +492,10 @@ export class AsyncIterator<T> extends EventEmitter {
@returns {module:asynciterator.AsyncIterator} A new iterator that appends items to this iterator
*/
append(items: T[] | AsyncIterator<T>): AsyncIterator<T> {
return this.transform({ append: items });
return new UnionIterator(
[this, Array.isArray(items) ? new ArrayIterator(items, { autoStart: false }) : items],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

const sources = this._pending!.sources!;
delete this._pending!.sources;
// @ts-ignore
this._sources = (Array.isArray(sources) ? fromArray(sources) : wrap(sources)).map<AsyncIterator<T>>((it: any) => isPromise(it) ? wrap(it as any) : (it as AsyncIterator<T>));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could also be simplified after #63

private _currentSource = -1;
export class UnionIterator<T> extends AsyncIterator<T> {
private _sources : AsyncIterator<AsyncIterator<T>>;
private buffer = new LinkedList<AsyncIterator<T>>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a _ prefix.

mutateFilter(filter: (item: V) => boolean) {
let last: LinkedNode<V> | null;
let next: LinkedNode<V> | null;
while (this._head !== null && !filter(this._head.value)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this could be simplified into one single loop if we were to have the list itself implement the LinkedNode interface, referencing the first node in the chain as next instead of _head. It might not be worth the effort, though.

@jeswr
Copy link
Collaborator Author

jeswr commented Apr 20, 2022

Thanks for the review - I'll wait for #63 to be merged and the rebase and clean this up :)

private _sources : InternalSource<T>[] = [];
private _pending? : { sources?: AsyncIterator<MaybePromise<AsyncIterator<T>>> };
private _currentSource = -1;
export class UnionIterator<T> extends AsyncIterator<T> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document whether or not it is in-order; crucial to prepend and append.

@jeswr
Copy link
Collaborator Author

jeswr commented Aug 1, 2022

Did work getting this up to date on the train. Will push changes later today

jeswr referenced this pull request in comunica/comunica Aug 1, 2022
jeswr added a commit that referenced this pull request Aug 1, 2022
@jeswr jeswr mentioned this pull request Aug 1, 2022
@jeswr jeswr closed this Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance: Improve performance of append and prepend
3 participants