Skip to content

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
@satorg satorg deleted the oscar/20250102_chain_traverseVoid branch July 6, 2025 02:04
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.

4 participants