Skip to content

Optimize Chain.traverseVoid #4695

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

Merged
merged 4 commits into from
Jan 10, 2025
Merged

Conversation

johnynek
Copy link
Contributor

@johnynek johnynek commented Jan 3, 2025

we can directly iterate through traverseVoid (which is really a kind of monoidal aggregation), so we don't need to use the expensive foldRight to do this.

@satorg satorg added the behind-the-scenes appreciated, but not user-facing label Jan 3, 2025
satorg
satorg previously approved these changes Jan 3, 2025
@satorg
Copy link
Contributor

satorg commented Jan 3, 2025

The optimization looks great, thank you!

However, I have to say that it seems that traverseVoid for Chain has never been really covered by any tests (unless I missed something). The only "exception" I managed to find is this case in ReducibleLaws:

+ NonEmptyChain[Int] with Option: nonEmptyTraverse.nonEmptyTraverseVoid consistent with traverseVoid

which apparently is not very comprehensive.

There's also abstract TraverseSuite implemented for a generic F[_], but currently ChainSuite does not inherit it.

Therefore, perhaps the simplest way to address that could be inheriting ChainSuite from TraverseSuite. It wouldn't check all possible outcomes though, but I guess it would be better than the way it is now.

danicheg
danicheg previously approved these changes Jan 3, 2025
Copy link
Member

@danicheg danicheg left a comment

Choose a reason for hiding this comment

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

Overall, LGTM

Just wondering, previously, if the effect wasn't a stack-safe Monad, we specifically used foldRight + Eval as a safety-net. Isn't this a thing anymore?

case Append(l, r) =>
go(l, if (rhs.isEmpty) r else Append(r, rhs), acc)
case Wrap(as) =>
val va = Traverse[Vector].traverseVoid(as.toVector)(f)
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to see the difference in memory consumption though

@johnynek
Copy link
Contributor Author

johnynek commented Jan 3, 2025

@danicheg

about:

Just wondering, previously, if the effect wasn't a stack-safe Monad, we specifically used foldRight + Eval as a safety-net. Isn't this a thing anymore?

There are two ways the stack can overflow: due to some recursion on the chain as we are traversing, or via the effect that we are combining with. foldRight + Eval can prevent a stack overflow from recursion on the chain, but inside it is still using map2Eval which has a default implementation of just calling map2 inside of an Eval. The original motivation of map2Eval was to allow short circuiting when the lhs of the map2 was already a failure, (e.g. None in Option, or Left in Either, or Invalid in Validated, etc...). But when there isn't a failure, you still have to evaluate the rhs, so it will have to build the linear chain of what amounts to G.product(a0.void, G.product(a1.void, ...

So, I don't see how stack unsafety of repeated product applications can be improved with the current main version.

Additionally, Chain isn't a linear chain necessarily. It can have some treelike structure, so it can only have G.product depth <= the original depth, never more.

@johnynek johnynek dismissed stale reviews from danicheg and satorg via fe40b32 January 3, 2025 18:28
@johnynek
Copy link
Contributor Author

johnynek commented Jan 3, 2025

okay, I followed the existing convention for List/Vector etc... and just added a new class to the TraverseSuite file.

How does that look?

@johnynek johnynek merged commit 304ab00 into main Jan 10, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behind-the-scenes appreciated, but not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants