Improve memory usage in Ref.Atomic #9710
Conversation
|
|
134b828 to
007fded
Compare
007fded to
22bbe71
Compare
|
Not sure which behaviour for
private[zio] final class Atomic[A](initial: A) extends Ref[A] { self =>
private val initialStr = initial.toString
...
override def toString: String = s"Ref.Atomic(initial = $initialStr)"
...
}While this could be useful in some scenarios, depending on
override def toString: String = s"Ref.Atomic(${unsafe.get(Unsafe)})"This fixes the issue and is arguably a more useful
override def toString: String = s"Ref.Atomic"Needless to say, this is the least useful version, but at least it certainly doesn't bring any problems with memory or calling unsafe code outside of zio Please let me know what option you prefer, or maybe there is a better way to fix it that I can't see 👀 |
|
Thanks for the contribution, and fixing my mistake. I think either behavior is fine (with a bias to no value shown) - perhaps @kyri-petrou or @ghostdogpr have an opinion on the behavior. |
| @@ -359,7 +359,7 @@ object Ref extends Serializable { | |||
| override def updateSomeAndGet(pf: PartialFunction[A, A])(implicit trace: Trace): UIO[A] = | |||
| ZIO.succeed(unsafe.updateSomeAndGet(pf)(Unsafe)) | |||
|
|
|||
| override def toString: String = s"Ref.Atomic(initial = $initial)" | |||
| override def toString: String = s"Ref.Atomic" | |||
There was a problem hiding this comment.
What value is printed if we don't override?
There was a problem hiding this comment.
It's just the default java implementation that prints hash, so like zio.Ref$Atomic@2d97c38c. This actually seems better than what I put now 😅 So let's not override (and remove the test)? To be fair, the more I think about it, the less sense it makes to override toString here in the first place. Like if you want to see the current value (as it was prior your fix), just do .get and do .toString on that, this way you suspend everything that needs to be suspended. Expecting Ref#toString to print some content of the wrapped value is odd
There was a problem hiding this comment.
Yeah, I agree. We can just remove the override + test.
Fixes #9709