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

Optimise NonEmptyTraverse implementation #3382

Conversation

gagandeepkalra
Copy link
Contributor

resolves #3381

depends on #3375

Comment on lines +463 to 480
tail.headOption.fold(Eval.now(Apply[G].map(f(head))(NonEmptyLazyList(_)))) { h =>
Apply[G].map2Eval(f(head), Eval.defer(loop(h, tail.tail)))((b, acc) => b +: acc)
}
Copy link
Contributor

@diesalbla diesalbla Apr 19, 2020

Choose a reason for hiding this comment

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

There is a bit of LISP here. This code may be more readable as follows:

Suggested change
tail.headOption.fold(Eval.now(Apply[G].map(f(head))(NonEmptyLazyList(_)))) { h =>
Apply[G].map2Eval(f(head), Eval.defer(loop(h, tail.tail)))((b, acc) => b +: acc)
}
val fh = f(head)
tail match {
case _ if tail.isEmpty =>
Eval.now(Apply[G].map(fh)(NonEmptyLazyList(_)))
case h #:: ttail =>
val ftail = Eval.defer(loop(h, ttail))
Apply[G].map2Eval(fh, ftail)(_ +: _)
}

The changes are:

  • Extract f(head), which is eagerly evaluated in both branches, as a val.
  • Replace the Option.fold by a pattern-match.
  • Replace the cases of headOption by two cases on the tail lazy list itself. For the first case, there seems not to be any case-object, like Nil, for the empty list. Discussion.
  • In the second case, abbreviate (b, acc) => b +: acc by underscores _ +: _.

Copy link
Contributor Author

@gagandeepkalra gagandeepkalra Jun 2, 2020

Choose a reason for hiding this comment

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

Hello, I see what you mean there.

On my part, usually I try to avoid pattern match (it's slow)

@gagandeepkalra gagandeepkalra force-pushed the test/nonEmptyTraverse/short-circuiting branch from 9c6e999 to fbecfba Compare May 26, 2020 01:55
@codecov-commenter
Copy link

codecov-commenter commented May 26, 2020

Codecov Report

Merging #3382 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3382      +/-   ##
==========================================
- Coverage   91.63%   91.60%   -0.03%     
==========================================
  Files         382      382              
  Lines        8304     8311       +7     
  Branches      232      226       -6     
==========================================
+ Hits         7609     7613       +4     
- Misses        695      698       +3     

LukaJCB
LukaJCB previously approved these changes May 26, 2020
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thanks @gagandeepkalra :)

@travisbrown travisbrown self-requested a review June 2, 2020 09:27
travisbrown
travisbrown previously approved these changes Jun 2, 2020
Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

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

This looks good to me! Do you mind rebasing to resolve the conflicts, @gagandeepkalra? I can merge it after that.

@gagandeepkalra gagandeepkalra dismissed stale reviews from travisbrown and LukaJCB via dd3c3bc June 2, 2020 12:18
@gagandeepkalra gagandeepkalra force-pushed the test/nonEmptyTraverse/short-circuiting branch from fbecfba to dd3c3bc Compare June 2, 2020 12:18
@gagandeepkalra gagandeepkalra force-pushed the test/nonEmptyTraverse/short-circuiting branch from 6dbb35a to 50a0cf9 Compare June 2, 2020 12:28
@gagandeepkalra
Copy link
Contributor Author

Thank you @travisbrown @LukaJCB, this was fun. I'm happy knowing short-circuiting is already useful (I see foldable there).

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thanks a ton!

Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

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

Thanks again, @gagandeepkalra!

@travisbrown travisbrown merged commit 0ade708 into typelevel:master Jun 4, 2020
@gagandeepkalra gagandeepkalra deleted the test/nonEmptyTraverse/short-circuiting branch June 4, 2020 13:57
@travisbrown travisbrown added this to the 2.2.0-M3 milestone Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimise NonEmptyTraverse implementation
5 participants