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

Use SAM syntax for typeclass instances where possible #4279

Merged
merged 13 commits into from
Sep 6, 2022

Conversation

joroKr21
Copy link
Member

@joroKr21 joroKr21 commented Aug 11, 2022

Definitions are shorter and don't generate anonymous classes in some cases.

Instances:

  • Show
  • Eq
  • PartialOrder
  • Order
  • Semigroup, CommutativeSemigroup
  • Monoid, CommutativeMonoid
  • Band
  • Semilattice, BoundedSemilattice

Closes #3870

@joroKr21 joroKr21 force-pushed the sam-instances branch 2 times, most recently from e10b935 to 9d0d5f4 Compare August 11, 2022 14:56
new BoundedSemilattice[A] {
def empty: A = self.zero
def combine(x: A, y: A): A = join(x, y)
}
BoundedSemilattice.instance(zero, join)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really a SAM, so isn't it subject to my same concern from #3871 (comment)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the concern was when there are more than one lambda

Copy link
Member

@armanbilge armanbilge Aug 12, 2022

Choose a reason for hiding this comment

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

Hmm, not sure why this is different. I think it is storing two references (zero and join) where it previously stored one (a reference to self)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but is that really a problem when before we used to have an entire new class instead of an additional reference?

Copy link
Member

Choose a reason for hiding this comment

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

Except, you have one class per JVM vs an extra reference per instance :) FTR I don't particularly care about this, I'm just unconvinced whether we are really optimizing anything here. See my comment in #3871 (comment).

Additionally, isn't this de-optimizing the current implementations? By replacing dedicated classes with lambdas and thus adding a level of indirection and a larger per-instance footprint. Like, these are micro-optimizations, but isn't saving a few megabytes? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well if it's not worth it then we can also close this PR and the related issue.
One minor advantage is that the definitions get a bit more concise.

@armanbilge armanbilge added this to the 2.9.0 milestone Aug 12, 2022
@joroKr21 joroKr21 marked this pull request as ready for review August 12, 2022 17:50
@johnynek
Copy link
Contributor

I'm all for using the sam syntax for things like val eqBool: Eq[Boolean] = { (a, b) => a == b } since (I believe) that is the same performance at runtime.

However, this is doing something different since we are not taking a function literal and returning a trait value, we are passing a literal to a function so it must have the indirection.

It may be inlined by the JIT, but my understanding is that Function1 being megamorphic that will not happen. So I'm concerned this is a performance degradation for a slight benefit (perhaps) in jar size and in lines of code.

@joroKr21
Copy link
Member Author

I'm all for using the sam syntax for things like val eqBool: Eq[Boolean] = { (a, b) => a == b } since (I believe) that is the same performance at runtime.

Do you think we should do that instead? I would be in favour of that change.

@johnynek
Copy link
Contributor

yeah, I think that change would be fine.

Another idea, we could add a macro that we could write on 2.12/2.13 and 3.0 that would work like some of these constructor methods except we know that it will inline. So this could be useful for a more concise Monoid implementation for instance (since it isn't a SAM).

But honestly, that sounds like a lot of work for some pretty minor syntax wins.

@armanbilge
Copy link
Member

I'm all for using the sam syntax for things like val eqBool: Eq[Boolean] = { (a, b) => a == b } since (I believe) that is the same performance at runtime.

Yes, I believe it is equivalent in performance, but IIUC it still generates an anonymous class in Scala 2 due to the compiler bug described in #3871 (comment).

It may be inlined by the JIT, but my understanding is that Function1 being megamorphic that will not happen. So I'm concerned this is a performance degradation for a slight benefit (perhaps) in jar size and in lines of code.

Yup, my concern too, which I raised in #3871 (comment). There at least there were significant decreases in jar size.

@joroKr21 joroKr21 changed the title Use helper constructors for SAM typeclass instances Use SAM syntax for typeclass instances where possible Aug 13, 2022
@joroKr21
Copy link
Member Author

Ok I made the changes to use SAM syntax

johnynek
johnynek previously approved these changes Aug 24, 2022
@johnynek
Copy link
Contributor

nice to see so many net lines removed!

danicheg
danicheg previously approved these changes Aug 25, 2022
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.

Monumental work.

@joroKr21 joroKr21 dismissed stale reviews from danicheg and johnynek via ab74f33 August 25, 2022 15:45
@johnynek johnynek merged commit 9aa75b7 into typelevel:main Sep 6, 2022
@joroKr21 joroKr21 deleted the sam-instances branch September 7, 2022 07:31
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.

Use helper constructors to instantiate type classes
5 participants