Skip to content
This repository has been archived by the owner on Feb 20, 2019. It is now read-only.

Force generation to create (un)picklers together #412

Merged
merged 3 commits into from
Apr 18, 2016

Conversation

jvican
Copy link
Member

@jvican jvican commented Apr 17, 2016

  • Macros generate Pickler[T]/Unpickler[T] together instead of
    creating them separately. This reduces final code size since now
    there's only one class instead of two and also improves the reuse of
    already generated picklers (see issue Cache already generated picklers for known types #409).
  • As now the return type of the implicit method is
    AbstractPicklerUnpickler, there can be some errors because of
    ambiguous implicit values (e.g. sealed-trait-static) when (un)picklers
    are explicitly called via generate. The fix is to only use
    PicklerUnpickler.generate and deprecate separate explicit creation
    with Pickler.generate and Unpickler.generate.
  • Make Any and Either to generate pickler/unpickler together.
  • Improve documentation by adding more concrete explanations.

* Make `Any` and `Either` to generate pickler/unpickler at the same
  time.

* Improve some documentation by adding more concrete explanations.
@jvican
Copy link
Member Author

jvican commented Apr 17, 2016

Also, it's worth mentioning the fact that we reduce considerably the sourcegen code (by 2/3), which is good for future maintainability of the project.

@jvican jvican force-pushed the generate-pickler-unpickler-together branch 2 times, most recently from 3ffb457 to 3347bbd Compare April 17, 2016 18:44
@jvican
Copy link
Member Author

jvican commented Apr 17, 2016

I've discovered some issues with this PR, don't merge (yet) please.

* Macros generate `Pickler[T]`/`Unpickler[T]` together instead of
  creating them separately. This reduces final code size since now
  there's only one class instead of two and also improves the reuse of
  already generated picklers (see issue scala#409).

* As now the return type of the implicit method is
  `AbstractPicklerUnpickler`, there can be some errors because of
  ambiguous implicit values (e.g. sealed-trait-static) when (un)picklers
  are explicitly called via `generate`. The fix is to only use
  `PicklerUnpickler.generate` and deprecate separate explicit creation
  with `Pickler.generate` and `Unpickler.generate`.
@jvican jvican force-pushed the generate-pickler-unpickler-together branch from 3347bbd to 7d3a7ac Compare April 18, 2016 09:30
* When generating (un)picklers, most of the time we look for the pickler
  of `Any`. By exposing the pickler of `Any`, we don't manually create
  one pickler/unpickler every time it's needed. This speeds up the
  compilation time almost by a factor of 2.
@jvican jvican force-pushed the generate-pickler-unpickler-together branch from 7d3a7ac to 48e1c4d Compare April 18, 2016 11:17
@@ -26,7 +26,8 @@ trait Pickler[T] {
// Shim for Java code.
abstract class AbstractPickler[T] extends Pickler[T]
object Pickler {
def generate[T]: Pickler[T] = macro generator.Compat.genPickler_impl[T]
@deprecated("Use `PicklerUnpickler.generate` instead", "0.11")
def generate[T]: Pickler[T] = macro generator.Compat.genPicklerUnpickler_impl[T]
Copy link
Contributor

Choose a reason for hiding this comment

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

yay!

@jsuereth
Copy link
Contributor

jsuereth commented Apr 18, 2016

Yeah, the sourgen reduction is nice. I like where we're going. a few notes:

  1. Either in this PR (or open a ticket so we remember): We should update the runtime registry of picklers/unpicklers so that it works on PicklerUnpicklers rather than the individual ones. This includes generating both when we need runtime creation of picklers.
  2. we should get a name for PicklerUnpickler

In any case, great work! Really awesome to see all these cleanups!!

@jvican
Copy link
Member Author

jvican commented Apr 18, 2016

This can already be merged, but there's no way to share/reuse already generated picklers/unpicklers so far and I don't know if it's going to be possible either. Implicits never get out of the scope. The only way to reuse them is this one:

implicit val pu = PicklerUnpickler.generate[T]

Note the implicit keyword in the definition of pu. This only can be done in concrete cases. Currently, the way the algorithm proceeds is super inefficient, since it's creating (un)picklers for every call site, and repeating the generation of a LOT of picklers/unpicklers (including the nested ones). However, it's not sure if there's another alternative.

My last commit explains how exposing an AnyPickler speeds up the compilation time by 40% in my computer (the generation of a pickler for any is crazily requested when compiling the test suite). However, there's room for improvement, and we should try something to reuse the generated picklers/unpicklers.

@jsuereth
Copy link
Contributor

Agree on having AnyPickler be there by default. I think because we DON'T have contravariant picklers and covariant unpicklers (by necessity) this is fine.

Relating to avoiding generating the code, for PicklerUnpickler.generate, the implicit bit is the best we can do.

At some point I'd love to be able to have something like:

sealed trait Bar
@Autopicklers object Bar

@Autopicklers
final case class Foo....

Where we can autogenerate the implicit val for the type. Right now though, I think having implicit-val in companion obejcts isn't the worst.

@jsuereth
Copy link
Contributor

If you think you're ready on this PR, it's good t merge. Looks good!

@jvican
Copy link
Member Author

jvican commented Apr 18, 2016

Yes, that's exactly what I thought, the only way AnyPickler could be chosen is if it we explicitly pickle something which is Any. And it that's the case, it's the user's fault.

Yes, let's merge this, I'll move this conversation to a ticket so that we can make a serious/formal proposal.

@jvican
Copy link
Member Author

jvican commented Apr 18, 2016

BTW, I was also thinking about that macro annotation. It's a great idea although it changes the design and the API. But we would really get good improvements. Right now, the code size is bloated of unnecessary repetitive picklers/unpicklers.

@jsuereth jsuereth merged commit c04a1ab into scala:0.11.x Apr 18, 2016
@jvican jvican added this to the 0.11.0-M1 milestone May 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants