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

alleycats - Extract[F[_]] the only way to get the syntax is the deprecated one #4670

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

aluscent
Copy link

Fixes #4642

Comment on lines 30 to 32
implicit class ExtractOps[F[_], A](fa: F[A])(implicit ev: Extract[F]) {
def extract(): A = ev.extract(fa)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes sense to convert it into a value class, e.g.:

Suggested change
implicit class ExtractOps[F[_], A](fa: F[A])(implicit ev: Extract[F]) {
def extract(): A = ev.extract(fa)
}
implicit final class ExtractOps[F[_], A](private val fa: F[A]) extends AnyVal {
def extract(implicit ev: Extract[F]): A = ev.extract(fa)
}

The reason why I think it could make sense is that

  1. This is a pretty valid use case for value classes in general and allows to save on memory allocations a little bit.

  2. Newer syntaxes in Cats-core use this approach a lot, see Applicative syntax for example:

    final class ApplicativeIdOps[A](private val a: A) extends AnyVal {
    def pure[F[_]](implicit F: Applicative[F]): F[A] = F.pure(a)
    }
    @deprecated("Use by-value or by-name version", "2.8.0")
    final class ApplicativeOps[F[_], A](private val fa: F[A]) extends AnyVal {
    def replicateA(n: Int)(implicit F: Applicative[F]): F[List[A]] = F.replicateA(n, fa)
    def unlessA(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.unlessA(cond)(fa)
    def whenA(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.whenA(cond)(fa)
    }
    final class ApplicativeByValueOps[F[_], A](private val fa: F[A]) extends AnyVal {
    def replicateA(n: Int)(implicit F: Applicative[F]): F[List[A]] = F.replicateA(n, fa)
    def replicateA_(n: Int)(implicit F: Applicative[F]): F[Unit] = F.replicateA_(n, fa)
    }
    final class ApplicativeByNameOps[F[_], A](private val fa: () => F[A]) extends AnyVal {
    def unlessA(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.unlessA(cond)(fa())
    def whenA(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.whenA(cond)(fa())
    }

@satorg
Copy link
Contributor

satorg commented Oct 31, 2024

Thank you for the PR!

@aluscent
Copy link
Author

aluscent commented Nov 2, 2024

Great idea! I would definitely apply that.
I also asked a question about test cases in the issue's thread. Please check it out.

The reason why I think it could make sense is that:
1. This is a pretty valid use case for value classes in general and allows to save on memory allocations a little bit.
2. Newer syntaxes in Cats-core use this approach a lot, see Applicative syntax for example:

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

aluscent commented Nov 2, 2024

The problem here is that the code should be implemented in this way:

object extract extends ExtractSyntax

trait ExtractSyntax {
  implicit def toExtractOps[F[_], A](fa: F[A]): ExtractOps[F, A] = new ExtractOps[F, A](fa)
}

final class ExtractOps[F[_], A](private val fa: F[A]) extends AnyVal {
  def extract(implicit ev: Extract[F]): A = ev.extract(fa)
}

Doesn't this break the limitations of using value classes? I mean passing type arguments to ExtractOps and the new ExtractOps[F, A](fa) all will result in unintended boxing, right?

@aluscent aluscent requested a review from satorg November 5, 2024 08:45
@satorg
Copy link
Contributor

satorg commented Nov 11, 2024

@aluscent ,
sorry for the long reply.

Doesn't this break the limitations of using value classes? I mean passing type arguments to ExtractOps and the new ExtractOpsF, A all will result in unintended boxing, right?

To be honest, I'm not sure I'm totally getting your concern. Why do you think the above should cause the boxing, could you elaborate please? To my best knowledge, no it shouldn't in this case. But I might be missing something. So do not take my word for it :)

If you're in doubt, you can check it for yourself by decompiling scala code to java (there are a plenty of open-source tools available) and see what it looks like. I did it a few times for some other typeclasses and AnyVal worked out pretty good there (i.e. no extra allocation/boxing whatsoever), but those might not be exactly the same case as this one.

@aluscent
Copy link
Author

Dear @satorg, I also have to apologize for answering so late. I was busy bringing up a campaign and in the meantime, I was checking the decompiled code to see if the boxing occurs.
This is the closest I could've get to the code you mentioned (to avoid boxing):

object extract extends ExtractSyntax

trait ExtractSyntax {
  implicit final def catsSyntaxExtract[F[_], A](fa: F[A]): ExtractOps[F, A] = new ExtractOps[F, A](fa)
}

final class ExtractOps[F[_], A](private val fa: F[A]) extends AnyVal {
  def extract(implicit F: Extract[F]): A = F.extract(fa)
}

This piece of code while preserving the implicit ExtractOps, makes it a value class.
Although the efforts, boxing was inevitable in any way I tried. I put the decompiled code down below. Here's the code belonging to extract.class:

public final class extract {
    public static Object catsSyntaxExtract(final Object fa) {
        return extract$.MODULE$.catsSyntaxExtract(fa);
    }
}

public final class extract$ implements ExtractSyntax {
    public static final extract$ MODULE$ = new extract$();

    static {
        ExtractSyntax.$init$(MODULE$);
    }

    public final <F, A> F catsSyntaxExtract(final F fa) {
        return ExtractSyntax.catsSyntaxExtract$(this, fa);
    }

    private extract$() {
    }
}

And the code for ExtractSyntax.class:

public interface ExtractSyntax {
    // $FF: synthetic method
    static Object catsSyntaxExtract$(final ExtractSyntax $this, final Object fa) {
        return $this.catsSyntaxExtract(fa);
    }

    default <F, A> F catsSyntaxExtract(final F fa) {
        return fa;
    }

    static void $init$(final ExtractSyntax $this) {
    }
}

Also the code for ExtractOps.class:

public final class ExtractOps<F, A> {
   private final F alleycats$syntax$ExtractOps$$fa;

   public static <F, A> boolean equals$extension(final F $this, final Object x$1) {
      return .MODULE$.equals$extension($this, x$1);
   }

   public static <F, A> int hashCode$extension(final F $this) {
      return .MODULE$.hashCode$extension($this);
   }

   public static <F, A> A extract$extension(final F $this, final Extract<F> F) {
      return .MODULE$.extract$extension($this, F);
   }

   public F alleycats$syntax$ExtractOps$$fa() {
      return this.alleycats$syntax$ExtractOps$$fa;
   }

   public A extract(final Extract<F> F) {
      return .MODULE$.extract$extension(this.alleycats$syntax$ExtractOps$$fa(), F);
   }

   public int hashCode() {
      return .MODULE$.hashCode$extension(this.alleycats$syntax$ExtractOps$$fa());
   }

   public boolean equals(final Object x$1) {
      return .MODULE$.equals$extension(this.alleycats$syntax$ExtractOps$$fa(), x$1);
   }

   public ExtractOps(final F fa) {
      this.alleycats$syntax$ExtractOps$$fa = fa;
   }
}

public final class ExtractOps$ {
   public static final ExtractOps$ MODULE$ = new ExtractOps$();

   public final <F, A> A extract$extension(final F $this, final Extract<F> F) {
      return F.extract($this);
   }

   public final <F, A> int hashCode$extension(final F $this) {
      return $this.hashCode();
   }

   public final <F, A> boolean equals$extension(final F $this, final Object x$1) {
      boolean var10000;
      if (x$1 instanceof ExtractOps) {
         Object var5 = x$1 == null ? null : ((ExtractOps)x$1).alleycats$syntax$ExtractOps$$fa();
         if (BoxesRunTime.equals($this, var5)) {
            var10000 = true;
            return var10000;
         }
      }

      var10000 = false;
      return var10000;
   }

   private ExtractOps$() {
   }
}

Now with my knowledge of Java, this code results in boxing in several points:

  • The class ExtractOps[F, A] is defined as a value class (extends AnyVal) in Scala, but due to its generic nature (F), the JVM cannot avoid boxing in certain cases. Generics in the JVM are erased at runtime, so ExtractOps[F, A] is effectively treated as an Object, forcing the JVM to allocate an instance.
  • Methods like extract in ExtractOps are compiled into extension methods within the ExtractOps$ singleton. These methods operate on the wrapped value (fa), and because $this is generic, boxing occurs whenever F is instantiated as a primitive type.

And many other points.

I am studying and trying to find ways to make sure, and this comment is for updating you and the team. Please let me know if you know better ways to make sure.

@satorg
Copy link
Contributor

satorg commented Nov 23, 2024

@aluscent , thank you for the incredible research!

Did you try to make a call to the 'extract' extension, decompile it, and find out what the code looks like at the call site?

The fact that compiler generates ExtractOps class and other stuff does not tell exactly that it will be instantiated in every case.

@aluscent
Copy link
Author

aluscent commented Dec 2, 2024

@satorg I tested the code the way you told me and as you've suggested, it doesn't result in boxing. I will add the tests ASAP and add to this pull request.
Thanks for your help!

@aluscent
Copy link
Author

aluscent commented Dec 7, 2024

Dear @satorg
The changes are done. Please check the code and let me know if it needs further changes.

Copy link
Contributor

@satorg satorg left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@aluscent
Copy link
Author

@satorg I can't wait to see this gets merged. Will it happen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

alleycats - Extract[F[_]] the only way to get the syntax is the deprecated one
2 participants