Add lazy initialization and isLookupEnabled #411

jvican · 2016-04-15T11:32:23Z

Now, generated picklers/unpicklers are initialised only if there is no
pickler/unpickler already registered for a given type. If that's the
case, the registered on is reused.
There's a special handling for the generation of both Pickler and
Unpickler. We have to make sure that both pickler/unpickler exist and
that they are the same one, since we have to return a class that
matches the return type (Pickler[T] with Unpickler[T]).
As a side note, it's necessary to cast Unpicklers and
Pickler with Unpickler to Generated, otherwise the implicit search
fails.
Add a field in the PicklerRegistry that check if the lookup is
enabled for every kind of registr.

jvican · 2016-04-15T11:33:27Z

It depends on #408 and therefore contains its commit messages as well. When the parent is merged, I will rebase. Please, check only the other two last commits.

jvican · 2016-04-15T11:58:37Z

Failed tests are somehow related to shareNothing. I don't really know what's going on here, since @jsuereth has pointed out in the past some problems with shareNothing and refPicklers in the new release.

The tests are expecting memory-related exceptions that don't get to be thrown but correctly (un)pickled. The following comment explains that there were some errors thrown by Jenkins: https://github.com/scala/pickling/blob/0.11.x/core/src/test/scala/pickling/run/share-json.scala#L64.

Interestingly, all the tests pass if run using Intellij.

jvican · 2016-04-15T12:05:04Z

No, actually these tests should fail, there's a circular dependency and shareNothing is in scope. So I guess that this is uncovering a new issue with it, isn't it?

jvican · 2016-04-15T12:23:06Z

My guess of what's happening: Intellij and sbt runs tests differently. Somehow, with sbt, the tests share some global state. Therefore, it is first executed and registered the (un)pickler for C which was created with shareEverything in scope. Later, when the (un)pickler for C that uses shareNothing is invoked, it reuses the registered one.

EDIT: My guess has been confirmed. Working on a fix for this.

jsuereth · 2016-04-15T13:49:53Z

core/src/main/scala/scala/pickling/internal/DefaultPicklerRegistry.scala

@@ -52,7 +52,15 @@ final class DefaultPicklerRegistry(generator: RuntimePicklerGenerator) extends P
  override def registerUnpickler[T](key: String, p: Unpickler[T]): Unit =
    unpicklerMap.put(key, p)

-  /** Checks the existince of an unpickler. */
+  /** Checks the existence of a pickler ignoring the registered generators. */
+  override def lookupExistingPickler(key: String): Option[Pickler[_]] =


Note: Generators are for type constructors and SHOULD be safe to call. They do not involve runtime reflection.

E.g. List[_] is impossible to register a pickler for, but you can register a pickler that handles it if it can lookup a pickler for element types.

Note: I may be adding statically generated "generator" functions for types like List[_] where you leave some of the type constructor args "empty" and we get them passed in at runtime and do a runtime lookup (but not runtime reflection).

In any case, you shoudl be using lookupPickler/lookupUnpickler.

Yes, I know they do not involve runtime reflection. But we want that a concrete pickler has preference over the generator. That is to say that Pickler[List[String]] should be returned and registered instead of returning a generator Pickler[List[_]]. That's why these methods exist: we want to check when we initialize concrete (un)picklers that there is no other concrete (un)pickler for that type. This was done with #410 on mind, thus those tests will pass.

jsuereth · 2016-04-15T13:57:45Z

NICE! Generally LGTM, but one caveat: Tests are failing.

This is another instance where because we have a global runtime registry of picklers, you'll need to "swap" or "clear" the register in the test to make sure we can generate a new pickler with sharing disabled.

It may make sense to add a "clear" method to to the pickler registry just for testing. You could probably make it private[pickling] so that only our tests can call it. I'm not sure it makes sense in other contexts....

Option #2 is to implement a thread-local pickler registry and swap threads. That seems pretty crazy and not a good idea....

jvican · 2016-04-15T14:02:16Z

I think there is even a better solution: to keep track of what picklers/unpicklers are generated with SharedNothing and check in the lazy initialization if there's a mismatch between the Shared options of the pickler that we want to initialize and the one that is registered.

I'll add the clear method but I believe that my solution is more focused on the long term, because other similar issues could appear in production code (I've thought of other use cases in which this could fail).

WDYT? BTW, I'm working on this right now, i'll have it in 10 min or so.

jsuereth · 2016-04-15T14:16:32Z

@jvican Well, considering I'm thinking of deprecating the way we handle sharing (because it's subtly broken for most statically generated picklers), I'm not sure we want to expose more sharing-related code :)

That said, tracking in the pickler whether or not it supports sharing is probably best. That said, if you have one pickler with sharing and another without in the same runtime, isn't that a logic bug in your program?

jvican · 2016-04-15T14:21:16Z

It depends... going again to the analogy of client-server, wouldn't be good to support the use case of having a client that fully knows the message that is sending (and therefore enables shareNothing) and a server (who doesn't know anything about the message received, but knows that it could be either a tree or graph, and therefore enables shareEverything)?

Also, it's not necessary to keep track of that in the (un)picklers, we can do it just in the PicklerRegistry by asking for an implicit of Share. I think it's pretty neat and it's not a lot of code, so we can remove it whenever you tackle the refactoring of the sharing 😄

jsuereth · 2016-04-15T14:32:48Z

@jvican So the client/server analogy falls apart here. The issue only arises same JVM. The reasons tests fail is those tests do teh following (in sequence):

Register sharing picklers
pickle and validate
Register non-sharing picklers (oops, we can't do that now)
pickle and validate

So for testing reasons, being able to "clean" the registered picklers between tests may be useful. Eitehr that Or when we test non-sharing we create totally new types/classees. That's probably the easiest (and least hacky) solution :)

jvican · 2016-04-15T14:40:25Z

Your suggestion of creating totally new types for non-sharing (un)picklers is great. I think that it's very similar to my current solution (which I'm about to submit) but I don't embed that "sharing" information in the type, just in a dictionary.

Tests are passing now in my machine. Otherwise, if you prefer to strictly follow the 'embed-in-the-types' strategy I can give it a try!

* Add a field in the `PicklerRegistry` that check if the lookup is enabled for every kind of registr.

* Now, generated picklers/unpicklers are initialised only if there is no pickler/unpickler already registered for a given type. If that's the case, the registered on is reused. * There's a special handling for the generation of both Pickler and Unpickler. We have to make sure that both pickler/unpickler exist and that they are the same one, since we have to return a class that matches the return type (`Pickler[T]` with `Unpickler[T]`). * As a side note, it's necessary to cast `Unpickler`s and `Pickler with Unpickler` to `Generated`, otherwise the implicit search fails.

jsuereth · 2016-04-15T14:44:35Z

let me see what you have, if it's almost done, then I'll comment :) Hopefully it's good to go.

* Because of lazy initialization, checks are not necessary anymore.

jsuereth · 2016-04-15T14:53:01Z

core/src/main/scala/scala/pickling/internal/DefaultPicklerRegistry.scala

+  private val unpicklerMap: Mmap[String, Unpickler[_]] = new TrieMap[String, Unpickler[_]]
+  private val unpicklerGenMap: Mmap[String, UnpicklerGenerator] = new TrieMap[String, UnpicklerGenerator]
+
+  private val shareNothingPicklers: Mset[String] = Mset.empty[String]


Given that most users will be picking ONE share strategy for their entire protocol, I do think this is a pretty big overhead/burden on the API/users for such a small use case (specifically the test).

jsuereth · 2016-04-15T14:53:47Z

@jvican SO, I think pickling strategies are already complicated enough. I think maybe we just fix the tests so the "share nothing" types are not the same as the 'share' tests...

jvican · 2016-04-15T14:55:32Z

I agree, working on the clear solution!

jvican · 2016-04-15T15:38:43Z

Tests are passing with the clear trick.

jsuereth · 2016-04-15T15:50:14Z

core/src/main/scala/scala/pickling/spi/PicklerRegistry.scala

@@ -98,5 +98,7 @@ trait PicklerRegistry {
    *                   this type.
    */
  def registerPicklerUnpicklerGenerator[T](typeConstructorKey: String, generator: FastTypeTag[_] => (Pickler[T] with Unpickler[T])): Unit
-  // TODO - Some kind of clean or inspect what we have?
+
+  private[pickling] def clearRegisteredPicklerUnpicklerFor[T: FastTypeTag]: Unit


nit: Can you add some quick docs explaining this? We understand, but someone new to the codebase will be curious.

Totally forgot 😄

jsuereth · 2016-04-15T15:50:35Z

One minor nit, fix that and feel free to merge! looking great.

* Different generated picklers with opposite sharing strategies are created and only the first one is registered. The second one never gets initiliazed and reuses the first one. * Remove `freshName` since scala `2.10.0` doesn't support.

jvican · 2016-04-16T13:02:22Z

Tests passed. Merging this!

jsuereth reviewed Apr 15, 2016
View reviewed changes

jvican added 2 commits April 15, 2016 16:43

Allow isLookupEnabled in the PicklerRegistry

86679f2

* Add a field in the `PicklerRegistry` that check if the lookup is enabled for every kind of registr.

Don't check existence in AutoRegister

0c83b97

* Because of lazy initialization, checks are not necessary anymore.

jvican force-pushed the lazy-init-generated-picklers-unpicklers branch 3 times, most recently from 4147b37 to 07c28cf Compare April 15, 2016 14:47

jsuereth reviewed Apr 15, 2016
View reviewed changes

jvican force-pushed the lazy-init-generated-picklers-unpicklers branch 2 times, most recently from f44ae1e to b1aa05b Compare April 15, 2016 15:32

jsuereth reviewed Apr 15, 2016
View reviewed changes

Fix clash between picklers with different sharing

0d03bcf

* Different generated picklers with opposite sharing strategies are created and only the first one is registered. The second one never gets initiliazed and reuses the first one. * Remove `freshName` since scala `2.10.0` doesn't support.

jvican force-pushed the lazy-init-generated-picklers-unpicklers branch from b1aa05b to 0d03bcf Compare April 16, 2016 12:48

jvican merged commit 31af63c into scala:0.11.x Apr 16, 2016

jvican mentioned this pull request Apr 16, 2016

Add tests that show #409 #410

Merged

jvican added this to the 0.11.0-M1 milestone May 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lazy initialization and isLookupEnabled #411

Add lazy initialization and isLookupEnabled #411

jvican commented Apr 15, 2016

jvican commented Apr 15, 2016

jvican commented Apr 15, 2016 •

edited

Loading

jvican commented Apr 15, 2016

jvican commented Apr 15, 2016 •

edited

Loading

jsuereth Apr 15, 2016

jvican Apr 15, 2016

jsuereth commented Apr 15, 2016

jvican commented Apr 15, 2016

jsuereth commented Apr 15, 2016

jvican commented Apr 15, 2016

jsuereth commented Apr 15, 2016

jvican commented Apr 15, 2016

jsuereth commented Apr 15, 2016

jsuereth Apr 15, 2016

jsuereth commented Apr 15, 2016

jvican commented Apr 15, 2016

jvican commented Apr 15, 2016

jsuereth Apr 15, 2016

jvican Apr 16, 2016

jsuereth commented Apr 15, 2016

jvican commented Apr 16, 2016

Add lazy initialization and isLookupEnabled #411

Add lazy initialization and isLookupEnabled #411

Conversation

jvican commented Apr 15, 2016

jvican commented Apr 15, 2016

jvican commented Apr 15, 2016 • edited Loading

jvican commented Apr 15, 2016

jvican commented Apr 15, 2016 • edited Loading

jsuereth Apr 15, 2016

Choose a reason for hiding this comment

jvican Apr 15, 2016

Choose a reason for hiding this comment

jsuereth commented Apr 15, 2016

jvican commented Apr 15, 2016

jsuereth commented Apr 15, 2016

jvican commented Apr 15, 2016

jsuereth commented Apr 15, 2016

jvican commented Apr 15, 2016

jsuereth commented Apr 15, 2016

jsuereth Apr 15, 2016

Choose a reason for hiding this comment

jsuereth commented Apr 15, 2016

jvican commented Apr 15, 2016

jvican commented Apr 15, 2016

jsuereth Apr 15, 2016

Choose a reason for hiding this comment

jvican Apr 16, 2016

Choose a reason for hiding this comment

jsuereth commented Apr 15, 2016

jvican commented Apr 16, 2016

jvican commented Apr 15, 2016 •

edited

Loading

jvican commented Apr 15, 2016 •

edited

Loading