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

In Apply.semigroup test replace ListWrapper with Option #2827

Merged
merged 3 commits into from
May 4, 2019

Conversation

kailuowang
Copy link
Contributor

This is to address the root cause of #2648 and #2319, the target method of this test is the combine method provided by Apply.semigroup
https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Apply.scala#L223-L225

def combine(a: F[A], b: F[A]): F[A] =
    f.map2(a, b)(sg.combine)

In the case of F being a list, since this is a product operation between the two lists, the result is a list of the size a.size * b.size
When this combine is used in the combineAllOption test, which combine all items in a Vector[ListWrapper[Int]], it exponentially increase the size of the accumulative result list. That leads to a list of millions of items and over limit GC pressure that breaks the build.

I switch it to using Option instead.

@kailuowang kailuowang added this to the 2.0.0-RC1 milestone May 3, 2019
@codecov-io
Copy link

codecov-io commented May 3, 2019

Codecov Report

Merging #2827 into master will decrease coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2827     +/-   ##
=========================================
- Coverage   94.17%   93.87%   -0.3%     
=========================================
  Files         368      368             
  Lines        6933     6955     +22     
  Branches      303      305      +2     
=========================================
  Hits         6529     6529             
- Misses        404      426     +22
Impacted Files Coverage Δ
core/src/main/scala/cats/Apply.scala 75% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 422c989...bfcf98c. Read the comment docs.

rossabaker
rossabaker previously approved these changes May 3, 2019
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Looks good, but see suggestion on scaladoc.

* This semigroup uses a product operation to combine `F`s,
* if the `Apply[F].product` result in larger `F` e.g. when `F` is a `List`,
* accumulative usage of this instance, such as `combineAll`, will result in
* `F`s with exponentially increasing sizes.
Copy link
Member

Choose a reason for hiding this comment

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

Light copy editing (spans lines, so I couldn't GitHub it):

   * This semigroup uses a product operation to combine `F`s.
   * If the `Apply[F].product` results in larger `F` (i.e. when `F` is a `List`),
   * accumulative usage of this instance, such as `combineAll`, will result in
   * `F`s with exponentially increasing sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

@DavidGregory084
Copy link
Member

Thanks for looking into that @kailuowang!

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.

Thankfully we can eradicate one more source of flimsyness in the tests!

@kailuowang kailuowang merged commit 9b6a6fd into typelevel:master May 4, 2019
@kailuowang kailuowang modified the milestones: 2.0.0-RC1, 2.0.0-M2 Jun 4, 2019
@kailuowang kailuowang deleted the improve-tests branch April 22, 2020 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants