scala / pickling

Fast, customizable, boilerplate-free pickling support for Scala
lampwww.epfl.ch/~hmiller/pickling
BSD 3-Clause "New" or "Revised" License
831 stars 79 forks source link

Scala pickle creates needless copies of objects #2

Closed markehammons closed 11 years ago

markehammons commented 11 years ago

In this code

object Test extends App {
        val x = new X
        val p = x.pickle
        val y = p.unpickle[X]
        x.a(0) = 4
        y.a(0) = 4
        println(x.b.mkString) //prints 423
        println(y.b.mkString) //prints 123
}

class X {
        var a = Array(1,2,3)
        var b = a
}

X will pickle into:

JSONPickle({
  "tpe": "X",
  "a": [
    1,
    2,
    3
  ],
"b": [
  1,
  2,
  3
]
})

Which will depickle into a X with two copies of the same array. In the above example, y is a depickled x. Instead of b pointing to the same array object as a, it now points to a new, different array.

This is related to issue #1

heathermiller commented 11 years ago

I understand the relationship with #1 and the issue of the additional object being added to the pickle.

However, I don't understand how you're getting this behavior: X.b(0) == 4?

As far as I can see, it has to be X.b(0) == 1.

Example:

import scala.pickling._
import json._

object Main extends App {
  class X {
    var a = List(1,2,3)
    var b = a
  }

  val x = new X
  println(x.b(0))

  val p = x.pickle
  println(p.value)

  val u = p.unpickle[X]
  println(u.b(0))
}

Correctly Results in:

1
{
  "tpe": "Main.X",
  "a": {
    "tpe": "scala.collection.immutable.$colon$colon[scala.Int]",
    "elems": [
      1,
      2,
      3
    ]
  },
  "b": {
    "tpe": "scala.collection.immutable.$colon$colon[scala.Int]",
    "elems": [
      1,
      2,
      3
    ]
  }
}
1

That is b(0) is 1, not 4. Did I miss something? I'd just like to ensure something isn't semantically amiss with the pickle format.

heathermiller commented 11 years ago

It also seems that in the pickle you pasted into the issue description, you might have an array, not a list? Our JSONPickled output looks a bit different...

Even if I change X's fields to be an array instead of a list, I still get the correct expected output:

1
{
  "tpe": "Main.X",
  "a": [
    1,
    2,
    3
  ],
"b": [
  1,
  2,
  3
]
}
1
markehammons commented 11 years ago

Yeah, I meant an array. The output contains the correct values, but the serialization should have some way of indicating that a and b are references to the same object, so that you don't get slightly different data when you serialize classes like these.

heathermiller commented 11 years ago

Yep, as mentioned I understand that– though you earlier mentioned some additional semantically incorrect behavior X.b(0) = 4. The issue description has since been edited, so I take it that all is OK, semantically, with accessing the 0th element of an unpickled instance of member b of X? That is, that point is a non-issue, right?

Good thing is that, as mentioned in related ticket #1, we're currently working to make pickling cyclic object graphs possible and more convenient– so both #1 and #2 should be resolved once our fix is pushed. :-)

markehammons commented 11 years ago

Yep, as mentioned I understand that– though you earlier mentioned some additional semantically incorrect behavior X.b(0) = 4.

Yeah I meant to say what I'm now showing with the example code. Sorry I wasn't clear. Thanks a lot :+1:

heathermiller commented 11 years ago

Ah, yeah totally missed that the example code changed too! Sorry! Alright, awesome, all clear now, thanks! :)

phaller commented 11 years ago

As commented on https://github.com/scala/pickling/issues/1, this has been resolved in the "oopsla2013" branch (https://github.com/scala/pickling/tree/oopsla2013) and will be integrated into the main branches soon.

heathermiller commented 11 years ago

All of this is now in the main 2.10.x branch, so marking as fixed :)