#9638 Make Cause.toString stack-safe #9853
Conversation
| /** | ||
| * Stack-safe toString for Cause | ||
| */ | ||
| def causeToString[E](cause: Cause[E]): String = { |
There was a problem hiding this comment.
WDUT about adding this within the abstract class and make it final private (and removing the cause: Cause[E] argument ofc)?
There was a problem hiding this comment.
The idea moving the method to companion Object was to avoid using Cause successors within abstract class. IMO it's more correct. But in fact successors are already mentioned within Cause so let me move it
Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>
| if (visitStack.isEmpty) { | ||
| results.pop() | ||
| } else { | ||
| visitStack.pop() match { |
There was a problem hiding this comment.
You can actually avoid the either entirely by just matching on Function1 or Cause - though I am not if it matters since these seem unlikely to retain.
| visitRecursive() | ||
| } | ||
|
|
||
| final override def toString = causeToString |
There was a problem hiding this comment.
@kyri-petrou What do you think about avoiding the stack allocations if we just have Die or Fail?
There was a problem hiding this comment.
Good point (also for Empty and Interrupt)
There was a problem hiding this comment.
I've remade it: moved the function to trait and applied it to all "composite" causes
| visitRecursive() | ||
| } | ||
|
|
||
| final override def toString = causeToString |
There was a problem hiding this comment.
Good point (also for Empty and Interrupt)
| visitStack.push(Left(() => results.push(s"Stackless(${results.pop()},$stackless)")), Right(cause)) | ||
| case Die(value, trace) => | ||
| results.push(s"Die($value,$trace)") | ||
| case Empty => |
There was a problem hiding this comment.
Oh I missed this previously. Avoid using case Empty because it uses the equals method of Cause which can be expensive. This is much more performant as it relies on type-checking alone:
| case Empty => | |
| case _: Empty.type => |
There was a problem hiding this comment.
Remade all checks
| /** | ||
| * A Cause that contains one or more sub-causes | ||
| */ | ||
| sealed trait CompositeCause[+E] { self: Cause[E] => |
There was a problem hiding this comment.
the problem with this approach is that users can now match on this - increasing the API we have to support for binary compatibility. I think we should unfortunately write:
def tostring = self match {
case _: Empty => "Empty"
case _:Die | _:Fail _ => super.toString // not sure if this one is correct
case default => compositToString
@kyri-petrou what do you think?
There was a problem hiding this comment.
I don't think super.toString will work because the compiler implements the toString method of case classes, it's not due to some super class.
In either way, I think we should go with the simplest solution possible. The toString method of Cause shouldn't be getting used normally (it's not used in logging, only in ZIO#debug which shouldn't be used in production code).
I know I was initially on-board with this change, but if this optimization requires code duplication we can revert the last commit and take the allocation hit because it should only really be used during development / debugging.
There was a problem hiding this comment.
@hearnadam if you agree I can make as propsed: revert last commit and apply only change that avoids using Either in stack
There was a problem hiding this comment.
Could we make the trait private[Cause]? that should be okay?
|
@kyri-petrou could you please review/merge PR? |
No description provided.