Use a dedicated typeclass to guide coproduct induction #924
Conversation
|
I will think about adding a test |
Codecov Report
@@ Coverage Diff @@
## master #924 +/- ##
==========================================
+ Coverage 81.96% 82.65% +0.69%
==========================================
Files 51 51
Lines 765 767 +2
Branches 28 36 +8
==========================================
+ Hits 627 634 +7
+ Misses 138 133 -5
Continue to review full report at Codecov.
|
|
Very interesting. How much of a compile time improvement we're talking about? |
joroKr21
changed the title
|
WIP for now as I want to see if shapeless has a builtin combinator we can use without sacrificing performance. Maybe something like In one case I measured compile time improvement from ~4min to ~6sec. |
|
Nice! Thanks for the extra details @joroKr21! Yeah, I'd be interested in seeing if Shapeless has anything we'd be able to reuse. Also, I'm not minding this thin type-class at all. Just let me know when you think it's ready for review. |
|
It seems like |
|
I'd personally be fine with this new type-class. Let's just make sure we get more people to take a look. |
| trait CoproductToResponse[C <: Coproduct] extends ToResponse[C] | ||
| object CoproductToResponse { | ||
| type Aux[C <: Coproduct, CT] = CoproductToResponse[C] { type ContentType = CT } | ||
| def apply[C <: Coproduct](implicit ctp: CoproductToResponse[C]): Aux[C, ctp.ContentType] = ctp |
There was a problem hiding this comment.
Should we drop this apply method? I don't see it's being used anywhere?
There was a problem hiding this comment.
Just habit, I will remove it.
| ): Aux[C, CT] = ctr | ||
| } | ||
|
|
||
| trait CoproductToResponse[C <: Coproduct] extends ToResponse[C] |
There was a problem hiding this comment.
I'd add an extra new line here.
| ): Aux[C, CT] = ctr | ||
| } | ||
|
|
||
| trait CoproductToResponse[C <: Coproduct] extends ToResponse[C] |
There was a problem hiding this comment.
Just an idea. How do you feel about moving this new typeclass under the ToResponse companion object? This way we can reduce the top-level/user-facing API surface. I'd imagine something like ToResponse.FromCoproduct should do it:
object ToReponse {
type Aux = ...
trait FromCoprodct extends ToResponse[...]
}
There was a problem hiding this comment.
Fine by me, I was wondering the same thing :)
There was a problem hiding this comment.
Unless you already working on it, I'd be happy to see this merged in its current state and then addressing this comment later. Whatever you prefer - just le me know.
|
LGTM 👍 |
|
It's surprising to me that this would have such a big impact on compile times, but 👍. |
|
FWIW, I tried compiling |
This disentangles the `Coproduct` induction logic in `ToResponse` derivation from the actual encoding of values. The net result is greatly reduced compilation times for large `Coproducts`.
joroKr21
force-pushed
the
coproduct-to-response
branch
from
fc583da
to
52e404f
Compare
joroKr21
changed the title
|
I addressed the comments. @vkostyukov as you noticed there is no easy way to test this. I published locally the changes here and noticed a tiny slowdown of about 10sec compared to the workaround I have in our codebase, but it's still a huge improvement wrt the baseline. The difference in the workaround is a specialized Anyway, I think it's fine like this. |
|
Love it! Thank you @joroKr21! |
This disentangles the
Coproductinduction logic inToResponsederivation from the actual encoding of values.
The net result is greatly reduced compilation times for large
Coproducts.Fixes #923