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

Derive typeclasses via Coercible hurts compiler performance? #64

Closed
Fristi opened this issue May 6, 2020 · 10 comments
Closed

Derive typeclasses via Coercible hurts compiler performance? #64

Fristi opened this issue May 6, 2020 · 10 comments

Comments

@Fristi
Copy link

Fristi commented May 6, 2020

Since the introduction of newtypes in conjuction with refined I see a big increase in compilation time. Especially in my postgres module. I was using refined before in my postgres module, but not with newtypes. I'm using doobie and SQL/fragment interpolated strings to construct queries. The string interpolator of doobie summons foreach interpolated variable a Put type class (which is used to convert the variable being interpolated to a JDBC data type).

Here's the code to convert newtypes to Put and Get (for reading data from JDBC)

  implicit def coerciblePut[N, P](implicit ev: Coercible[Put[P], Put[N]], R: Put[P]): Put[N] = ev(R)
  implicit def coercibleGet[N, P](implicit ev: Coercible[Get[P], Get[N]], R: Get[P]): Get[N] = ev(R)

I've included the scalac-profiling aswell: https://gist.github.com/Fristi/363e7fcf8f899e24eae53d42d741fd13

It seems to not have repeeated expansions, but rather unique expansions

@carymrobbins
Copy link
Member

While the example of using Coercible to auto-derive instances for all newtypes is provided in the README, I probably should have added a note that this is likely not a best practice. Honestly, I'm now thinking about removing it entirely.

I'm not sure if this helps your specific problem, but I would consider explicitly deriving instances for your types as the preferred way and not using the Coercible trick. One of the biggest reasons is that the Coercible trick puts you in a weird spot when you need to customize an instance. Yes, we could deal with implicit scoping rules, but I really try not to.

So I would recommend trying out using explicit instances and see if that helps. For example -

@newtype case class Text(s: String)
object Text {
  implicit val get: Get[Text] = deriving
  implicit val put: Put[Text] = deriving
}

It's certainly more code, but I tend to prefer the explicitness of this approach as opposed to the more magical one.

This very well might not help at all, if so, sorry for the derail, just something that seems worth bringing up given the problem; although, I do have a feeling that doing it the explicit way would be much easier on the compiler.

@carymrobbins
Copy link
Member

Oh, as a way to reduce boilerplate, you can get Haskell-style deriving in many cases using scalaz-deriving which has special support for @newtype -

@deriving(Get, Put)
@newtype case class Text(s: String)

@Fristi
Copy link
Author

Fristi commented May 7, 2020

Thanks for your extensive answer around this issue!

I've split up my code in modules, such that core types reside in a core module having no dependencies on doobie or circe whatsoever. Therefore I can't use deriving unfortunately.

However I've made some handcrafted instances with some replace tricks and shaved off 22 seconds of compilation in my postgres module, in my service module even 25 seconds.

Removing the Coercible trick for now then, it's unfortunate that this trick doesn't work that well in Scala due implicit resolution. Hopefully in Scala 3 this will be better.

Thanks for the great library by the way, cleaned up my code a lot :)

@Fristi Fristi closed this as completed May 7, 2020
@carymrobbins
Copy link
Member

Honestly, I tend to think about these things in a Haskell way, and if I were to do something like this -

instance (Coercible a b, Put a) => Put b where ...

it would (and should!) be pretty frowned upon. Instead, you'd just define the orphans explicitly and import them when you need them, which sounds like exactly what you are doing.

The pattern of having some core types module that takes near zero dependencies is a common one. It sounds like what you are doing now is exactly what I'd do in your situation, regardless of the existence of the Coercible trick. 😉

@Fristi
Copy link
Author

Fristi commented May 7, 2020

That's the trick I've learned in Haskell as well :) I didn't know that would be so bad? It saves a ton of code though.

Happy to have this resolved though, builds are much faster :)

@carymrobbins
Copy link
Member

That's the trick I've learned in Haskell as well :) I didn't know that would be so bad? It saves a ton of code though.

In Haskell you should probably prefer StandaloneDeriving with GeneralizedNewtypeDeriving

-- Package A
newtype Text = Text String

-- Package B
deriving newtype instance Put Text

Yes, it can be boilerplatey if you have a lot of them, but it's a bit safer this way. It might not always be the case that you wanted an instance for some particular newtype, or even that instance. Always better to define them explicitly. So in Scala, I recommend a similar strategy for the same reasons.

Happy to have this resolved though, builds are much faster :)

Fantastic!

@joroKr21
Copy link
Contributor

joroKr21 commented May 7, 2020

I wonder if this would have helped for the compile times though:

  implicit def coerciblePut[N, P](implicit ev: Coercible[P, N], R: Put[P]): Put[N] = R.coerce
  implicit def coercibleGet[N, P](implicit ev: Coercible[P, N], R: Get[P]): Get[N] = R.coerce

@carymrobbins
Copy link
Member

Why would that help? The only thing that looks different is the use of the extension method as opposed to just invoking apply(), which doesn't seem easier on the compiler.

@joroKr21
Copy link
Contributor

joroKr21 commented May 7, 2020

Idk it's hard to tell without looking at the codebase.
But perhaps eliminating implicit searches like these:

         "io.estatico.newtype.Coercible[doobie.util.Put[P],doobie.util.Put[courier.core.DepotName]]" -> 1,
         "io.estatico.newtype.Coercible[doobie.util.Put[P],doobie.util.Put[java.time.LocalDate]]" -> 1,
         "io.estatico.newtype.Coercible[doobie.util.Get[P],doobie.util.Get[courier.core.VacancyId.Type]]" -> 1,
         "io.estatico.newtype.Coercible[doobie.util.Put[P],doobie.util.Put[StringContext]]" -> 1,

@carymrobbins
Copy link
Member

Oh right, checking if P and N are Coercible does seem simpler than checking if the type class instance is Coercible.

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

No branches or pull requests

3 participants