From d90bed8565780fc1d0a3aff408ca9537a7d012ed Mon Sep 17 00:00:00 2001 From: Koert Kuipers Date: Mon, 11 Nov 2013 09:21:19 -0500 Subject: [PATCH 1/4] disable registration of 5 item map concrete class (is this safe on all platforms?) and add add check for class equivalence after serde rountrip --- .../scala/com/twitter/chill/ScalaKryoInstantiator.scala | 2 +- chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala b/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala index 80a04f29..7411ee23 100644 --- a/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala +++ b/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala @@ -135,7 +135,7 @@ class ScalaCollectionsRegistrar extends IKryoRegistrar { .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b)) .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c)) .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd)) - .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd, 'e -> 'e)) + //.forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd, 'e -> 'e)) // The normal fields serializer works for ranges .registerClasses(Seq(classOf[Range.Inclusive], classOf[NumericRange.Inclusive[_]], diff --git a/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala b/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala index 9ed53324..7e1b10f2 100644 --- a/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala +++ b/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala @@ -100,8 +100,7 @@ class KryoSpec extends Specification with BaseProperties { val rtTest = test map { serialize(_) } map { deserialize[AnyRef](_) } rtTest.zip(test).foreach { case (serdeser, orig) => serdeser must be_==(orig) - //orig.getClass.asInstanceOf[Class[Any]] must be_==(serdeser.getClass.asInstanceOf[Class[Any]]) - //println("orig " + orig.getClass.asInstanceOf[Class[Any]] + " => serde " + serdeser.getClass.asInstanceOf[Class[Any]]) + orig.getClass.asInstanceOf[Class[Any]] must be_==(serdeser.getClass.asInstanceOf[Class[Any]]) } } "round trip a SortedSet" in { @@ -213,8 +212,7 @@ class KryoSpec extends Specification with BaseProperties { val m2 = Map('a -> 'a, 'b -> 'b) val m3 = Map('a -> 'a, 'b -> 'b, 'c -> 'c) val m4 = Map('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd) - val m5 = Map('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd, 'e -> 'e) - Seq(m1, m2, m3, m4, m5).foreach { m => + Seq(m1, m2, m3, m4).foreach { m => rt(inst, m) must be_==(m) } } From ad430f07a5f509a21a61650b8e85ff302b9c1271 Mon Sep 17 00:00:00 2001 From: Koert Kuipers Date: Mon, 11 Nov 2013 09:29:19 -0500 Subject: [PATCH 2/4] add test for map with 5 items (since we removed the specific serializer for this small map) --- chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala b/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala index 7e1b10f2..a63bd43e 100644 --- a/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala +++ b/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala @@ -69,6 +69,7 @@ class KryoSpec extends Specification with BaseProperties { Left(Map(1->"YO!")), Some(Left(10)), Map("good" -> 0.5, "bad" -> -1.0), + Map('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd, 'e -> 'e), MArrayBuffer(1,2,3,4,5), List(Some(MHashMap(1->1, 2->2)), None, Some(MHashMap(3->4))), Set(1,2,3,4,10), From fb4722eb0200ad172203147a34b115aab4c06385 Mon Sep 17 00:00:00 2001 From: Koert Kuipers Date: Mon, 11 Nov 2013 12:38:11 -0500 Subject: [PATCH 3/4] remove commented line --- .../src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala b/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala index 7411ee23..07758fc0 100644 --- a/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala +++ b/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala @@ -135,7 +135,6 @@ class ScalaCollectionsRegistrar extends IKryoRegistrar { .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b)) .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c)) .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd)) - //.forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd, 'e -> 'e)) // The normal fields serializer works for ranges .registerClasses(Seq(classOf[Range.Inclusive], classOf[NumericRange.Inclusive[_]], From 9c55b71153dde5860c80a438ac5e1cc01ac32023 Mon Sep 17 00:00:00 2001 From: Koert Kuipers Date: Mon, 11 Nov 2013 16:08:14 -0500 Subject: [PATCH 4/4] register HashSet and HashMap concrete traversable instead of Set and Map (beyond specialized small size sets and maps). make sure to add the tests back in for registered map of size 5 (m5). add testing for class equivalence after serialization/deserialization roundtrip where applicable --- .../twitter/chill/ScalaKryoInstantiator.scala | 6 +- .../com/twitter/chill/BaseProperties.scala | 10 ++++ .../scala/com/twitter/chill/KryoSpec.scala | 58 +++++++++---------- 3 files changed, 42 insertions(+), 32 deletions(-) diff --git a/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala b/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala index 07758fc0..3feed9f8 100644 --- a/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala +++ b/chill-scala/src/main/scala/com/twitter/chill/ScalaKryoInstantiator.scala @@ -18,6 +18,7 @@ package com.twitter.chill import scala.collection.immutable.{ BitSet, + HashSet, ListSet, NumericRange, Range, @@ -129,12 +130,15 @@ class ScalaCollectionsRegistrar extends IKryoRegistrar { .forConcreteTraversableClass(Set[Any]('a, 'b)) .forConcreteTraversableClass(Set[Any]('a, 'b, 'c)) .forConcreteTraversableClass(Set[Any]('a, 'b, 'c, 'd)) - .forConcreteTraversableClass(Set[Any]('a, 'b, 'c, 'd, 'e)) + // default set implementation + .forConcreteTraversableClass(HashSet[Any]('a, 'b, 'c, 'd, 'e)) // specifically register small maps since Scala represents them differently .forConcreteTraversableClass(Map[Any, Any]('a -> 'a)) .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b)) .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c)) .forConcreteTraversableClass(Map[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd)) + // default map implementation + .forConcreteTraversableClass(HashMap[Any, Any]('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd, 'e -> 'e)) // The normal fields serializer works for ranges .registerClasses(Seq(classOf[Range.Inclusive], classOf[NumericRange.Inclusive[_]], diff --git a/chill-scala/src/test/scala/com/twitter/chill/BaseProperties.scala b/chill-scala/src/test/scala/com/twitter/chill/BaseProperties.scala index e182f4ff..e557d915 100644 --- a/chill-scala/src/test/scala/com/twitter/chill/BaseProperties.scala +++ b/chill-scala/src/test/scala/com/twitter/chill/BaseProperties.scala @@ -29,6 +29,16 @@ trait BaseProperties { pool.fromBytes(pool.toBytesWithClass(t)).asInstanceOf[T] } + def rtEquiv[T](t: T): Boolean = { + val serdeser = rt(t) + serdeser == t && serdeser.getClass.asInstanceOf[Class[Any]] == t.getClass.asInstanceOf[Class[Any]] + } + + def rtEquiv[T](k: KryoInstantiator, t: T): Boolean = { + val serdeser = rt(k, t) + serdeser == t && serdeser.getClass.asInstanceOf[Class[Any]] == t.getClass.asInstanceOf[Class[Any]] + } + // using java serialization. TODO: remove when this is shipped in bijection def jserialize[T <: Serializable](t: T): Array[Byte] = { val bos = new ByteArrayOutputStream diff --git a/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala b/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala index a63bd43e..f2dade91 100644 --- a/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala +++ b/chill-scala/src/test/scala/com/twitter/chill/KryoSpec.scala @@ -17,8 +17,9 @@ limitations under the License. package com.twitter.chill import org.specs._ +import org.specs.matcher.Matcher -import scala.collection.immutable.{SortedSet, BitSet, ListSet, SortedMap, ListMap, HashMap} +import scala.collection.immutable.{SortedSet, BitSet, ListSet, HashSet, SortedMap, ListMap, HashMap} import scala.collection.mutable.{ArrayBuffer => MArrayBuffer, HashMap => MHashMap} import _root_.java.util.PriorityQueue import _root_.java.util.Locale @@ -54,6 +55,10 @@ class KryoSpec extends Specification with BaseProperties { noDetailedDiffs() //Fixes issue for scala 2.9 + def roundtrip[T] = new Matcher[T]{ + def apply(t: => T) = (rtEquiv(t), "successfull serialization roundtrip for " + t, "failed serialization roundtrip for " + t) + } + def getKryo = KryoSerializer.registered.newKryo "KryoSerializers and KryoDeserializers" should { @@ -73,6 +78,7 @@ class KryoSpec extends Specification with BaseProperties { MArrayBuffer(1,2,3,4,5), List(Some(MHashMap(1->1, 2->2)), None, Some(MHashMap(3->4))), Set(1,2,3,4,10), + HashSet(1,2), SortedSet[Long](), SortedSet(1L, 2L, 3L, 4L), BitSet(), @@ -98,27 +104,24 @@ class KryoSpec extends Specification with BaseProperties { 'hai) .asInstanceOf[List[AnyRef]] - val rtTest = test map { serialize(_) } map { deserialize[AnyRef](_) } - rtTest.zip(test).foreach { case (serdeser, orig) => - serdeser must be_==(orig) - orig.getClass.asInstanceOf[Class[Any]] must be_==(serdeser.getClass.asInstanceOf[Class[Any]]) - } + test.foreach { _ must roundtrip } } "round trip a SortedSet" in { val a = SortedSet[Long]() // Test empty SortedSet val b = SortedSet[Int](1,2) // Test small SortedSet val c = SortedSet[Int](1,2,3,4,6,7,8,9,10)(Ordering.fromLessThan((x, y) => x > y)) // Test with different ordering - rt(a) must be_==(a) - rt(b) must be_==(b) + a must roundtrip + b must roundtrip + c must roundtrip (rt(c) + 5) must be_==(c + 5) } "round trip a ListSet" in { val a = ListSet[Long]() // Test empty SortedSet val b = ListSet[Int](1,2) // Test small ListSet val c = ListSet[Int](1,2,3,4,6,7,8,9,10) - rt(a) must be_==(a) - rt(b) must be_==(b) - (rt(c)) must be_==(c) + a must roundtrip + b must roundtrip + c must roundtrip } "handle trait with reference of self" in { var a= new ExampleUsingSelf{} @@ -126,9 +129,9 @@ class KryoSpec extends Specification with BaseProperties { b.count must be_==(1) } "handle manifests" in { - rt(manifest[Int]) must be_==(manifest[Int]) - rt(manifest[(Int,Int)]) must be_==(manifest[(Int,Int)]) - rt(manifest[Array[Int]]) must be_==(manifest[Array[Int]]) + manifest[Int] must roundtrip + manifest[(Int,Int)] must roundtrip + manifest[Array[Int]] must roundtrip } "handle arrays" in { def arrayRT[T](arr : Array[T]) { @@ -147,16 +150,14 @@ class KryoSpec extends Specification with BaseProperties { Array((1,1), (2,2), (3,3)).toSeq, Array((1.0, 1.0), (2.0, 2.0)).toSeq, Array((1.0, "1.0"), (2.0, "2.0")).toSeq) - tests.foreach { test => rt(test) must be_==(test) } + tests.foreach { _ must roundtrip } } "handle lists of lists" in { - val lol = List(("us", List(1)), ("jp", List(3, 2)), ("gb", List(3, 1))) - rt(lol) must be_==(lol) + List(("us", List(1)), ("jp", List(3, 2)), ("gb", List(3, 1))) must roundtrip } "handle scala singletons" in { - val test = List(Nil, None) - //Serialize each: - rt(test) must be_==(test) + List(Nil, None) must roundtrip + None must roundtrip (rt(None) eq None) must beTrue } "serialize a giant list" in { @@ -167,13 +168,11 @@ class KryoSpec extends Specification with BaseProperties { list2.zip(bigList).foreach { tup => tup._1 must be_==(tup._2) } } "handle scala enums" in { - WeekDay.values.foreach { v => - rt(v) must be_==(v) - } + WeekDay.values.foreach { _ must roundtrip } } "use java serialization" in { val kinst = { () => getKryo.javaForClass[TestCaseClassForSerialization] } - rt(kinst, TestCaseClassForSerialization("hey", 42)) must be_==(TestCaseClassForSerialization("hey", 42)) + rtEquiv(kinst, TestCaseClassForSerialization("hey", 42)) must beTrue } "work with Meatlocker" in { val l = List(1,2,3) @@ -213,9 +212,8 @@ class KryoSpec extends Specification with BaseProperties { val m2 = Map('a -> 'a, 'b -> 'b) val m3 = Map('a -> 'a, 'b -> 'b, 'c -> 'c) val m4 = Map('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd) - Seq(m1, m2, m3, m4).foreach { m => - rt(inst, m) must be_==(m) - } + val m5 = Map('a -> 'a, 'b -> 'b, 'c -> 'c, 'd -> 'd, 'e -> 'e) + Seq(m1, m2, m3, m4, m5).foreach { rtEquiv(inst, _) must beTrue } } "handle small immutable sets when registration is required" in { val inst = { () => @@ -228,9 +226,7 @@ class KryoSpec extends Specification with BaseProperties { val s3 = Set('a, 'b, 'c) val s4 = Set('a, 'b, 'c, 'd) val s5 = Set('a, 'b, 'c, 'd, 'e) - Seq(s1, s2, s3, s4, s5).foreach { s => - rt(inst, s) must be_==(s) - } + Seq(s1, s2, s3, s4, s5).foreach { rtEquiv(inst, _) must beTrue } } "handle nested mutable maps" in { val inst = { () => @@ -247,7 +243,7 @@ class KryoSpec extends Specification with BaseProperties { 1 -> mutable.Set("name3", "name4", "name1", "name2"), 0 -> mutable.Set(1, 2, 3, 4)) - rt(inst, obj0) must be_==(obj1) + rtEquiv(inst, obj0) must beTrue } "deserialize InputStream" in { val obj = Seq(1, 2, 3)