Fix unnecessary syntax allocation #4477

mox692 · 2023-07-11T19:04:17Z

Try to fix #4475.

core/src/main/scala/cats/syntax/apply.scala

mox692 · 2023-07-11T21:02:38Z

I plan to make modifications for syntax other than Apply in this PR as well. (but I may not be able to respond a bit until next week.)

core/src/main/scala/cats/syntax/apply.scala

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

armanbilge · 2023-07-23T14:34:57Z

core/src/main/scala/cats/syntax/apply.scala

+  implicit final def catsSyntaxProductOps[F[_], A, B](fa: F[A]): ProductOps[F, A, B] =
+    new ProductOps[F, A, B](fa)
+
+  implicit final def catsSyntaxMap2Ops[F[_], A, B, C](fa: F[A]): Map2Ops[F, A, B, C] =
+    new Map2Ops[F, A, B, C](fa)


Why separate these ones?

There is no strong reason to separate them, so I can put these together. (but it was a bit difficult to come up with a unique syntax name that doesn't conflict with exisisting one. ApplySyntaxFA or ApplyOps2 would be fine?)

ApplyOps2 or ApplyOpsBincompat1 or similar seems fine. We probably have some similar situations somewhere else in Cats.

Fix: f97affd

satorg · 2023-07-24T01:23:59Z

core/src/main/scala/cats/syntax/apply.scala

+  implicit final def catsSyntaxProductOps[F[_], A, B](fa: F[A]): ProductOps[F, A, B] =
+    new ProductOps[F, A, B](fa)


Oh, I doubt that type inference would work out well in this case. An implicit method should only include types that are parts of its parameters. Otherwise the compiler may not have a clue how to infer them. I would suggest

Suggested change

implicit final def catsSyntaxProductOps[F[_], A, B](fa: F[A]): ProductOps[F, A, B] =

new ProductOps[F, A, B](fa)

implicit final def catsSyntaxProductOps[F[_], A](fa: F[A]): YouNameIt[F, A] =

new YouNameIt[F, A](fa)

then in the YouNameIt:

final class YouNameIt[F[_], A](private val fa: F[A]) extends AnyVal { def productR[B](fb: F[B])(implicit F: Apply[F]): F[B] = F.productR(fa)(fb) }

I.e. the B type is introduced when it is actually used.
And the same for the other implicit methods below.

Feel free to give a try to your versions of course, but it looks suspicious to me personally.

And of course all the methods that require just F[A] can be grouped together. Only methods that require more specialized types like F[A => B] have to be extracted.

Oh, I didn't consider that aspect, thank you for pointing it out.

Fix: 0ee124e

mox692 · 2023-07-24T14:51:45Z

core/src/main/scala/cats/syntax/semigroupal.scala

+  @deprecated("Replaced by an apply syntax, e.g. instead of (a |@| b).map(...) use (a, b).mapN(...)", "1.0.0-MF")
+  def |@|[B](fb: F[B]): SemigroupalBuilder[F]#SemigroupalBuilder2[A, B] =
+    new SemigroupalBuilder[F] |@| fa |@| fb


This syntax is already deprecated, but I added it to the new syntax class for compatibility.

mox692 · 2023-07-24T15:04:48Z

I also tried to address ContravariantSemigroupal and ContravariantMonoidal mentioned in the issue, but I think the Ops that causing allocations doesn't provide any syntax (Or I may have missed something.)

cats/core/src/main/scala/cats/ContravariantSemigroupal.scala

Lines 59 to 63 in 7da28b4

    
           trait Ops[F[_], A] extends Serializable { 
        
             type TypeClassType <: ContravariantSemigroupal[F] 
        
             def self: F[A] 
        
             val typeClassInstance: TypeClassType 
        
           }

cats/core/src/main/scala/cats/ContravariantMonoidal.scala

Lines 65 to 69 in 7da28b4

    
           trait Ops[F[_], A] extends Serializable { 
        
             type TypeClassType <: ContravariantMonoidal[F] 
        
             def self: F[A] 
        
             val typeClassInstance: TypeClassType 
        
           }

Or should I deprecate the syntax that causes allocation and just add a new empty syntax class for future use?

core/src/main/scala/cats/syntax/semigroupal.scala

satorg · 2023-07-25T17:14:44Z

I also tried to address ContravariantSemigroupal and ContravariantMonoidal mentioned in the issue, but I think the Ops that causing allocations doesn't provide any syntax (Or I may have missed something.)
Or should I deprecate the syntax that causes allocation and just add a new empty syntax class for future use?

If there's no actual syntax that can be used then I would suggest simply to deprecate it.
I think it should not cause any source incompatibility in such a case.

Co-authored-by: Sergey Torgashov <satorg@users.noreply.github.com>

armanbilge

This looks good to me. Very nice work and thanks for your patience!

After we merge this it is probably time to release Cats v2.10.0 :)

satorg · 2023-08-02T15:46:24Z

core/src/main/scala/cats/syntax/apply.scala

 }

 private[syntax] trait ApplySyntaxBinCompat0 {
  implicit final def catsSyntaxIfApplyOps[F[_]](fa: F[Boolean]): IfApplyOps[F] =
    new IfApplyOps[F](fa)
 }

+final class ApplyFABOps[F[_], A, B](private val fab: F[A => B]) extends AnyVal with Serializable {


I feel we should either add with Serializable everywhere to each *Ops or go remove it, to be consistent.
However, this class does have it whereas the next one down below does not.
I personally don't see any reason why we would need Serializable for such a kind of object, so I'd prefer to remove it. But maybe there is a reason, @armanbilge could you suggest please?

See #4477 (comment). It shouldn't be needed if the syntax classes are not being allocated, which is the hope 😅

I don't know enough of the history but at some point it seems that people did care that these were serializable /shrug

It shouldn't be needed if the syntax classes are not being allocated

But even if those class instances are still allocated, what is the point in having them serialized whatsoever?
Those class instances are short-living and get instantiated just in place. They are not supposed to be saved nor transferred anywhere, are they?

They are not supposed to be saved nor transferred anywhere, are they?

I have no idea. I think typically serializability is important when you are dealing with e.g. Spark. Maybe in some (pathological) situations these classes can get caught up in the serialization graph somehow ...

🤔 yeah, not sure either. I would probably suggest to remove Serializable for now. Just because it is always easier to add something later rather than remove it whenever. We are not introducing any binary incompatibility anyway, so if Spark guys or anyone else start complaining, then we can easily issue a hotfix release. But at least we will know for sure in that case :) Although I'd guess there will be no complains about it.

Otherwise, if we decide to go the opposite way, then we should add Serializable all the remaining syntax extension classes, shouldn't we?

I'm fine with removing it. Anyway I think all Spark users are stuck on Cats 2.0.0-M5 or something 😛

Oh sorry, I unintentionally made ApplyFABOps Serializable🤦‍♂️
Thanks for pointing that out.

filipwiech · 2023-08-08T07:30:27Z

Hey @mox692, @armanbilge, @satorg! 🙂
Is there anything left to do here? Would be really great to get this merged and have Cats 2.10 released. The last release was a long while ago (Nov 22) and there have been several very nice improvements made in the meantime (including this one).
Thank you for your hard work on this, cheers! 👍

satorg · 2023-08-08T14:31:07Z

Sorry for the delay (and thank you for the ping 😄).

The code itself looks great indeed, no concerns about it. However I've found out that there're no tests that could help us to make sure that the changes do actually work as expected. It could be especially important for the syntax moved into the ApplyFABOps and ApplyFABCOps classes.

There are syntax tests in the SyntaxSuite class (see the tests module) that check particular syntaxes without any real code execution. I feel it would be really helpful if we could add the missing syntax calls into that test suite. It gets to be the testApply method in this case, I guess.

@mox692, wdyt?

danicheg · 2023-08-08T14:47:42Z

core/src/main/scala/cats/syntax/apply.scala

+  def <*>(fa: F[A])(implicit F: Apply[F]): F[B] = F.<*>[A, B](fab)(fa)
+}
+
+final class ApplyFABCOps[F[_], A, B, C](private val ff: F[(A, B) => C]) extends AnyVal {


Just came by to say this is a cool naming 😄

armanbilge · 2023-08-08T16:32:00Z

However I've found out that there're no tests that could help us to make sure that the changes do actually work as expected. It could be especially important for the syntax moved into the ApplyFABOps and ApplyFABCOps classes.

Ouch, good catch. I agree, we should have these to protect against these kind of refactorings.

need tests

mox692 · 2023-08-09T14:44:28Z

Sure, I'll add the test too.

mox692 · 2023-08-12T11:11:40Z

Hmmm, it looks like FutureSuite is failing in ci, but does this have anything to do with the change? 🤔

In mox692/cats, the test seems to succeed for the same commit 288587e.

ci logs

==> X cats.jvm.tests.FutureSuite.Future: coflatMap.coflatMap identity  3.022s munit.FailException: Failing seed: WznzS7nzhHoolYDQK4VxhrIMJ3M1xMQpRmki0cZs4_C=
You can reproduce this failure by adding the following override to your suite:

  override val scalaCheckInitialSeed = "WznzS7nzhHoolYDQK4VxhrIMJ3M1xMQpRmki0cZs4_C="

Exception raised on property evaluation.
> ARG_0: Future(Success(0))
> Exception: java.util.concurrent.TimeoutException: Future timed out after [3 seconds]
    at munit.ScalaCheckSuite.propToTry(ScalaCheckSuite.scala:98)
Caused by: java.util.concurrent.TimeoutException: Future timed out after [3 seconds]
    at scala.concurrent.impl.Promise$DefaultPromise.tryAwait0(Promise.scala:248)
    at scala.concurrent.impl.Promise$DefaultPromise.result(Promise.scala:261)
    at scala.concurrent.Await$.$anonfun$result$1(package.scala:201)
    at scala.concurrent.BlockContext$DefaultBlockContext$.blockOn(BlockContext.scala:62)
    at scala.concurrent.Await$.result(package.scala:124)
    at cats.jvm.tests.FutureSuite.cats$jvm$tests$FutureSuite$$$anonfun$eqfa$1(FutureSuite.scala:45)

armanbilge · 2023-08-13T18:00:34Z

Sorry, looks like it was a flake.

armanbilge

Thanks for adding those tests! Ok, I think we are finally good 😁

satorg

Thank you, looks great!

fix apply syntax allocation

99e9a71

mox692 marked this pull request as draft July 11, 2023 19:04

armanbilge reviewed Jul 11, 2023

View reviewed changes

core/src/main/scala/cats/syntax/apply.scala Show resolved Hide resolved

core/src/main/scala/cats/syntax/apply.scala Outdated Show resolved Hide resolved

core/src/main/scala/cats/syntax/apply.scala Outdated Show resolved Hide resolved

mox692 added 2 commits July 12, 2023 05:19

not remove original ops

af45152

fix ApplyBinCompat1 naming

ef328b8

remove implicit for F

0ba479b

armanbilge reviewed Jul 20, 2023

View reviewed changes

core/src/main/scala/cats/syntax/apply.scala Outdated Show resolved Hide resolved

mox692 and others added 4 commits July 21, 2023 00:09

Add deprecate annotation to old ops

0c642f6

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

create another syntax classes

e7d544f

Add doc comments

7ca6743

fix naming

2f5dc8f

armanbilge reviewed Jul 23, 2023

View reviewed changes

satorg reviewed Jul 24, 2023

View reviewed changes

mox692 added 4 commits July 24, 2023 20:39

Fix: implicit method type parameter

0ee124e

Fix: group together F[A] syntax

f97affd

Add: new Semigroupal syntax with no unnecessary allocations

e1ca164

Fix: failing doctest

5603fee

mox692 commented Jul 24, 2023

View reviewed changes

mox692 marked this pull request as ready for review July 24, 2023 15:04

armanbilge self-requested a review July 24, 2023 17:15

satorg self-requested a review July 24, 2023 17:30

Fix: failing doc test

fc6ac27

armanbilge added the bug label Jul 25, 2023

armanbilge added this to the 2.10.0 milestone Jul 25, 2023

satorg reviewed Jul 25, 2023

View reviewed changes

core/src/main/scala/cats/syntax/semigroupal.scala Outdated Show resolved Hide resolved

mox692 and others added 2 commits July 26, 2023 20:28

remove implicit from catsSyntaxSemigroupal

342996f

Co-authored-by: Sergey Torgashov <satorg@users.noreply.github.com>

Workaround to the failing test

649c38c

mox692 added 4 commits July 28, 2023 21:45

Uncurry implicits

0fbc093

Fix: typo

3f1e8e5

Tidying

f09b400

Remove unrelated diff

8ae9b9c

mox692 force-pushed the fix/unnecessary_syntax_allocation branch from 8ae9b9c to f09b400 Compare July 28, 2023 13:28

satorg requested a review from armanbilge July 28, 2023 17:32

armanbilge closed this Jul 31, 2023

armanbilge reopened this Jul 31, 2023

Merge ApplyOps and ApplyOps2

a0fdb0b

armanbilge previously approved these changes Aug 2, 2023

View reviewed changes

satorg reviewed Aug 2, 2023

View reviewed changes

armanbilge added 2 commits August 3, 2023 07:41

Remove spurious whitespace change

119f350

Remove Serializable from ApplyFABOps

5c06b8a

armanbilge dismissed their stale review via 5c06b8a August 3, 2023 07:43

armanbilge requested a review from satorg August 3, 2023 07:45

armanbilge previously approved these changes Aug 8, 2023

View reviewed changes

danicheg reviewed Aug 8, 2023

View reviewed changes

Add tests related to the change

288587e

mox692 requested a review from armanbilge August 12, 2023 10:46

armanbilge approved these changes Aug 13, 2023

View reviewed changes

satorg approved these changes Aug 13, 2023

View reviewed changes

armanbilge merged commit f496e25 into typelevel:main Aug 13, 2023
16 checks passed

armanbilge mentioned this pull request Aug 31, 2023

Update fs2-reactive-streams to 3.9.1 http4s/http4s-armeria#399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unnecessary syntax allocation #4477

Fix unnecessary syntax allocation #4477

mox692 commented Jul 11, 2023

mox692 commented Jul 11, 2023 •

edited

Loading

armanbilge Jul 23, 2023

mox692 Jul 24, 2023

armanbilge Jul 24, 2023

mox692 Jul 24, 2023

satorg Jul 24, 2023 •

edited

Loading

satorg Jul 24, 2023

mox692 Jul 24, 2023

mox692 Jul 24, 2023

mox692 commented Jul 24, 2023

satorg commented Jul 25, 2023

armanbilge left a comment

satorg Aug 2, 2023 •

edited

Loading

armanbilge Aug 2, 2023

satorg Aug 2, 2023 •

edited

Loading

armanbilge Aug 2, 2023

satorg Aug 2, 2023

armanbilge Aug 2, 2023

mox692 Aug 3, 2023 •

edited

Loading

filipwiech commented Aug 8, 2023

satorg commented Aug 8, 2023 •

edited

Loading

danicheg Aug 8, 2023

armanbilge commented Aug 8, 2023

mox692 commented Aug 9, 2023

mox692 commented Aug 12, 2023 •

edited

Loading

armanbilge commented Aug 13, 2023

armanbilge left a comment

satorg left a comment

		implicit final def catsSyntaxProductOps[F[_], A, B](fa: F[A]): ProductOps[F, A, B] =
		new ProductOps[F, A, B](fa)

Fix unnecessary syntax allocation #4477

Fix unnecessary syntax allocation #4477

Conversation

mox692 commented Jul 11, 2023

mox692 commented Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satorg Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mox692 commented Jul 24, 2023

satorg commented Jul 25, 2023

armanbilge left a comment

Choose a reason for hiding this comment

satorg Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satorg Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mox692 Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

filipwiech commented Aug 8, 2023

satorg commented Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

armanbilge commented Aug 8, 2023

mox692 commented Aug 9, 2023

mox692 commented Aug 12, 2023 • edited Loading

armanbilge commented Aug 13, 2023

armanbilge left a comment

Choose a reason for hiding this comment

satorg left a comment

Choose a reason for hiding this comment

mox692 commented Jul 11, 2023 •

edited

Loading

satorg Jul 24, 2023 •

edited

Loading

satorg Aug 2, 2023 •

edited

Loading

satorg Aug 2, 2023 •

edited

Loading

mox692 Aug 3, 2023 •

edited

Loading

satorg commented Aug 8, 2023 •

edited

Loading

mox692 commented Aug 12, 2023 •

edited

Loading