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

[NU-1094] Cats Effect 3 bump #4287

Merged
merged 47 commits into from Dec 19, 2023
Merged

[NU-1094] Cats Effect 3 bump #4287

merged 47 commits into from Dec 19, 2023

Conversation

lciolecki
Copy link
Contributor

@lciolecki lciolecki commented May 11, 2023

Describe your changes

  • bumped Cats Effect to version 3
  • refactoring
  • improved FlinkTestMainSpec
  • improved benchmarks

Benchmarks

Before

[info] Benchmark                                                                     Mode  Cnt        Score       Error  Units
[info] p.t.n.e.b.interpreter.ManyParamsInterpreterBenchmark.benchmarkAsync          thrpt   25   164289.415 ±  3444.184  ops/s
[info] p.t.n.e.b.interpreter.ManyParamsInterpreterBenchmark.benchmarkSync           thrpt   25   258857.290 ±  7074.453  ops/s
[info] p.t.n.e.b.interpreter.OneParamInterpreterBenchmark.benchmarkFutureAsync      thrpt   25    91131.231 ±   899.709  ops/s
[info] p.t.n.e.b.interpreter.OneParamInterpreterBenchmark.benchmarkFutureSync       thrpt   25  2317693.965 ± 73199.850  ops/s
[info] p.t.n.e.b.interpreter.OneParamInterpreterBenchmark.benchmarkAsyncIO          thrpt   25   147976.146 ±  8215.637  ops/s
[info] p.t.n.e.b.interpreter.OneParamInterpreterBenchmark.benchmarkSyncIO           thrpt   25  1392072.271 ± 16367.403  ops/s
[info] p.t.n.e.b.aggregate.AggregatorBenchamark.sumAggregatorAdd                     avgt   25        0.024 ±     0.001  us/op
[info] p.t.n.e.b.spel.SampleSpelBenchmark.benchmark                                  avgt   25        0.048 ±     0.001  us/op
[info] p.t.n.e.b.suggester.ExpressionSuggesterBenchmark.chainCallBenchmark             ss    5      101.173 ±    11.743  ms/op
[info] p.t.n.e.b.suggester.ExpressionSuggesterBenchmark.complexExpressionBenchmark     ss    5     1401.843 ±   278.441  ms/op

After

[info] Benchmark                                                                        Mode  Cnt        Score        Error  Units
[info] p.t.n.e.b.interpreter.ManyParamsInterpreterBenchmark.benchmarkAsyncService      thrpt   25   238584.487 ±   2240.850  ops/s
[info] p.t.n.e.b.interpreter.ManyParamsInterpreterBenchmark.benchmarkSyncService       thrpt   25   240782.340 ±   4590.133  ops/s
[info] p.t.n.e.b.interpreter.OneParamInterpreterBenchmark.benchmarkFutureAsyncService  thrpt   25   106851.712 ±   2253.305  ops/s
[info] p.t.n.e.b.interpreter.OneParamInterpreterBenchmark.benchmarkFutureSyncService   thrpt   25  1552035.161 ± 102814.218  ops/s
[info] p.t.n.e.b.interpreter.OneParamInterpreterBenchmark.benchmarkIOAsyncService      thrpt   25   102889.543 ±    294.573  ops/s
[info] p.t.n.e.b.interpreter.OneParamInterpreterBenchmark.benchmarkIOSyncService       thrpt   25   964319.320 ±   6763.407  ops/s
[info] p.t.n.e.b.aggregate.AggregatorBenchamark.sumAggregatorAdd                        avgt   25        0.024 ±      0.001  us/op
[info] p.t.n.e.b.spel.SampleSpelBenchmark.benchmark                                     avgt   25        0.049 ±      0.001  us/op
[info] p.t.n.e.b.suggester.ExpressionSuggesterBenchmark.chainCallBenchmark                ss    5       97.300 ±      9.947  ms/op
[info] p.t.n.e.b.suggester.ExpressionSuggesterBenchmark.complexExpressionBenchmark        ss    5     1209.948 ±     65.236  ms/op

Checklist before merge

  • Related issue ID is placed at the beginning of PR title in [brackets] (can be GH issue or Nu Jira issue)
  • Code is cleaned from temporary changes and commented out lines
  • Parts of the code that are not easy to understand are documented in the code
  • Changes are covered by automated tests
  • Showcase in dev-application.conf added to demonstrate the feature
  • Documentation added or updated
  • Added entry in Changelog.md describing the change from the perspective of a public distribution user
  • Added MigrationGuide.md entry in the appropriate subcategory if introducing a breaking change
  • Verify that PR will be squashed during merge

@lciolecki lciolecki force-pushed the improvements/bump-cats-effect branch 2 times, most recently from 5f16b1d to ddfe7f0 Compare May 11, 2023 12:27
@lciolecki lciolecki marked this pull request as draft May 11, 2023 12:31
@lciolecki lciolecki changed the base branch from release/1.9 to staging May 16, 2023 13:49
@lciolecki lciolecki force-pushed the improvements/bump-cats-effect branch from a2c2947 to dc0291d Compare May 23, 2023 20:03
@lciolecki lciolecki requested a review from piotrp May 25, 2023 17:33
@lciolecki lciolecki marked this pull request as ready for review May 25, 2023 17:33
@lciolecki lciolecki force-pushed the improvements/bump-cats-effect branch from 150106b to 87c6d4b Compare May 25, 2023 17:34
@github-actions github-actions bot added the docs label May 25, 2023
@lciolecki lciolecki force-pushed the improvements/bump-cats-effect branch 4 times, most recently from aba88c8 to d62419e Compare August 18, 2023 07:54
@lciolecki lciolecki changed the title Improvements: bump cats effect Improvement: bump cats effect Aug 18, 2023
@lciolecki lciolecki changed the title Improvement: bump cats effect Improvement: bump cats effect to 3.5.1 Aug 18, 2023
@lciolecki lciolecki force-pushed the improvements/bump-cats-effect branch from b651a3e to 72c623f Compare August 21, 2023 09:01
@lciolecki lciolecki marked this pull request as draft August 21, 2023 09:02
@coutoPL
Copy link
Member

coutoPL commented Aug 22, 2023

I think I managed to fix the problem found by FlinkTestMainSpec. Currently, this test is for the useIOMonadInInterpreter = true case. IMO it'd be nice to run the test with useIOMonadInInterpreter = false too. It seems that this case implementation was broken (now, it is fixed too).

@piotrp
Copy link
Member

piotrp commented Aug 22, 2023

@lciolecki can you recheck performance after these changes?

@coutoPL
Copy link
Member

coutoPL commented Aug 22, 2023

Based on the code of benchmarks we have, I think the previous implementation I pushed is not correct. It looks like we don't have good tests that cover the functionality we touch in this PR. Moreover, it looks like FlinkTestMainSpec needs some love too. Let me work on this for a while ...

@coutoPL coutoPL changed the title Improvement: bump cats effect to 3.5.1 [NU-1094] bump cats effect to 3.5.1 Dec 13, 2023
@coutoPL coutoPL marked this pull request as ready for review December 13, 2023 09:59
@coutoPL coutoPL changed the title [NU-1094] bump cats effect to 3.5.1 [NU-1094] Cats Effect 3 bump Dec 13, 2023
Copy link
Contributor

@raphaelsolarski raphaelsolarski left a comment

Choose a reason for hiding this comment

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

Good job!
I leaved a few minor comments.

Please, squash before merge 😄

@coutoPL coutoPL dismissed piotrp’s stale review December 19, 2023 10:07

Piotr is absent. His comments were addressed.

@coutoPL coutoPL merged commit 5fa9013 into staging Dec 19, 2023
17 checks passed
@coutoPL coutoPL deleted the improvements/bump-cats-effect branch December 19, 2023 10:07
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.

None yet

5 participants