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

should AsyncIterator.protoype.filter have a fast path for sync predicates? #230

Closed
michaelficarra opened this issue Sep 7, 2022 · 21 comments · Fixed by #239
Closed

should AsyncIterator.protoype.filter have a fast path for sync predicates? #230

michaelficarra opened this issue Sep 7, 2022 · 21 comments · Fixed by #239

Comments

@michaelficarra
Copy link
Member

As mentioned by @rbuckton in #117 (comment).

Also it was questioned whether true/false should be the fast path or truthy/falsey non-thenables. I would only support true/false.

@zloirock
Copy link
Contributor

zloirock commented Sep 7, 2022

Why only true / false? It can be at least all primitives - someone returns 0 or undefined as something falsy.

@devsnek
Copy link
Member

devsnek commented Sep 7, 2022

Given the choices:

  • always await
  • await if not true or false
  • await if IsPromise

I would choose always await, it seems to be the least chaotic.

@zloirock
Copy link
Contributor

zloirock commented Sep 7, 2022

@devsnek IsPromise works only with native promises - but it can be a userland implementation, so it's not an option.

@devsnek
Copy link
Member

devsnek commented Sep 7, 2022

@zloirock yes i agree.

@michaelficarra
Copy link
Member Author

@zloirock Yes we could support other primitives, but it gets more murky. What would the algorithm be? If it's an object, assume it's thenable, otherwise ToBoolean it? It would prevent weird things like putting a then method on String.prototype, but that's probably okay.

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

The algorithm would be if Type is Object, go the slow path. Seems reasonable to me.

@bakkot
Copy link
Collaborator

bakkot commented Sep 8, 2022

If we allow other primitives it's likely to run into weird timing issues. It's easy to image a predicate function which returns either an object or null, whcih would have an extra tick only in the null case. By contrast a predicate function which returns only true or false would have a consistent number of ticks in both cases.

On the other hand, maybe "number of ticks" is a thing not really worth caring about?

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

Only in that the desire is to minimize them in a predictable way, which i think “returns an object or null” achieves.

@devsnek
Copy link
Member

devsnek commented Sep 8, 2022

I think in these cases the more important thing is consistent behavior. this await will not be the "slow" part of real world code, the I/O from whatever async api is driving it will be.

@zloirock
Copy link
Contributor

zloirock commented Sep 8, 2022

.filter optimization is not the most interesting similar case. For example, why we should await the result of AsyncIterator.protoype.forEach sync callback that in many cases will return just undefined?

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

Because those need to be run in serial; filter operations do not.

@zloirock
Copy link
Contributor

zloirock commented Sep 8, 2022

Nope. All methods from this proposal should be run in serial, .filter too. I mean sync .forEach callback. If the result of the callback is undefined or another primitive, not promise. Similar to true of false for .filter from this issue.

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

What i mean is, conceptually, forEach is for side effects - filter is supposed to be pure.

Obviously they all need to run in order, but failing to await forEach would cause bugs in otherwise reasonable code - filter would not.

@bakkot
Copy link
Collaborator

bakkot commented Sep 8, 2022

It's not obvious to me how failing to await forEach would cause bugs in the specific case that the function passed to forEach returned undefined. We'd have to wait when it it returned a Promise, of course, but I don't see a problem (except for consistency) with avoiding the await undefined in the case that the function passed to forEach synchronously returns undefined.

@zloirock
Copy link
Contributor

zloirock commented Sep 8, 2022

...and the same situation with callbacks of almost all methods from this proposal - .map, .every, etc. - they are similar here and the resolution of this issue should be similar for all of them.

@rbuckton
Copy link

rbuckton commented Sep 8, 2022

@zloirock Yes we could support other primitives, but it gets more murky. What would the algorithm be? If it's an object, assume it's thenable, otherwise ToBoolean it? It would prevent weird things like putting a then method on String.prototype, but that's probably okay.

Putting a then on String.protoype doesn't turn "foo" into a thennable. The Await algorithm, and Promise resolution in general, only calls .then on Objects, per 27.2.1.3.2 Promise Resolve Functions, Step 8.

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

@bakkot actually yeah, good point. that suggests they can all have this fast path.

@rbuckton
Copy link

rbuckton commented Sep 8, 2022

I will point out that having a fast path is dangerously close to "releasing Zalgo". My original question was more to whether the result returned from the callback to filter should be awaited at all. I'm a bit wary about a conditional fast path, but I don't think it quite has the same consequence as the concerns described in the linked article.

@michaelficarra
Copy link
Member Author

@rbuckton Ah, I didn't know about that test. That makes calling ToBoolean on all primitives more reasonable, or at least consistent.

@zloirock
Copy link
Contributor

zloirock commented Sep 8, 2022

@rbuckton all methods of async iterators from this proposal that accept a callback accept async functions, I don't see any reason to make an exception only for .filter.

@rbuckton
Copy link

rbuckton commented Sep 8, 2022

Also, to my point about "releasing Zalgo", there is still an await that occurs prior to evaluating the filter callback (awaiting the result of next()), as well as an await prior to being able to observe the result (since the new iterator's next() also returns a Promise), so having a fast path to skip an extra await here isn't likely to be an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants