-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport fix made in #8765, improve readibility and optimize FiberRefs
stack implementation
#8779
Conversation
afff585
to
46576b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm too early to review when the PR is still in draft, but I was interested and wanted to take a look.
Feel free to ignore my comments if you were planning on doing more changes!
|
||
def combine(first: Patch, second: Patch): Patch = | ||
override final def combine(first: Patch, second: Patch): Patch = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just create a final private class FiberRefPatch
for this?
Personally I find reading stack traces & profiling data a lot more annoying with anonymous classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I find reading stack traces & profiling data a lot more annoying with anonymous classes
Completely agree :)
@@ -27,9 +28,8 @@ import scala.annotation.tailrec | |||
* example between an asynchronous producer and consumer. | |||
*/ | |||
final class FiberRefs private ( | |||
private[zio] val fiberRefLocals: Map[FiberRef[_], FiberRefs.Value] | |||
private[zio] val fiberRefLocals: Map[FiberRef[_], FiberRefStack[_]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick search of fiberRefLocals
across all zio/*
repos and it seems there's no other usage of it. Perhaps we can make it a private val
? That means we can also make a lot of things in the companion object private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try it. Thanks :)
@@ -248,7 +293,7 @@ object FiberRefs { | |||
* Applies the changes described by this patch to the specified collection | |||
* of `FiberRef` values. | |||
*/ | |||
def apply(fiberId: FiberId.Runtime, fiberRefs: FiberRefs): FiberRefs = { | |||
final def apply(fiberId: FiberId.Runtime, fiberRefs: FiberRefs): FiberRefs = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth for the last line in this method to be changed to this
if (self eq Empty) fiberRefs else loop(fiberRefs, List(self))
/** | ||
* Add a new entry on top of the Stack | ||
*/ | ||
@inline def put(fiberId: FiberId.Runtime, value: Any): FiberRefStack[?] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inline def put(fiberId: FiberId.Runtime, value: Any): FiberRefStack[?] = | |
@inline def put(fiberId: FiberId.Runtime, value: A): FiberRefStack[A] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, this change introduce a compilation error that I don't find how to solve:
[error] /Users/jules/workspace/zio/core/shared/src/main/scala/zio/FiberRefs.scala:72:38: type mismatch;
[error] found : newValue.type (with underlying type fiberRef.Value)
[error] required: _$2 where type _$2
[error] fiberRefStack.put(childId, newValue)
[error] ^
Any idea? I tried to explicitly cast newValue
but it doesn't solve the issue 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. Found how:
fiberRefStack.asInstanceOf[FiberRefs.FiberRefStack[fiberRef.Value]].put(childId, newValue)
…rRefs` stack implementation
Co-authored-by: kyri-petrou <[email protected]>
Co-authored-by: kyri-petrou <[email protected]>
Co-authored-by: kyri-petrou <[email protected]>
Co-authored-by: kyri-petrou <[email protected]>
Co-authored-by: kyri-petrou <[email protected]>
Co-authored-by: kyri-petrou <[email protected]>
Co-authored-by: kyri-petrou <[email protected]>
163a49e
to
ac0f395
Compare
@@ -1,11 +0,0 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO @guizmaii: Put back
Value(::(StackEntry(fiberId, value, 0), oldStack), oldDepth + 1) | ||
if (oldStack.fiberId == fiberId) { | ||
if (oldStack.value == value) oldStack else oldStack.updateValue(value) | ||
} else if (oldStack.value == value) oldStack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way is it just me or should this condition be checked first? It seems we check the same thing in L199 and L200, so there's not really any reason to check oldStack.fiberId == fiberId
when oldStack.value == value
@guizmaii hey 😄 do you think you'll be able to finish this one soon? I'm thinking of making one last 2.1.0 RC and then a final release from that RC. The question is, do I wait for this to be merged? It might be good to include it if there's a risk of regression or any behavior change. What do you think? |
@guizmaii let me know once you'd like me to review again |
Quick answer:
No, don't wait 🙂 Long answer:I'm not able to move forward much on this work because I have some health issues affecting quite negatively my everyday day life 😕 In this PR, I was exploring 2 things and was planning to explore a third thing later, related to the second thing I was exploring:
My plan was to write tests for the [error] Uncaught exception when running tests: Exception in thread "zio-fiber-2109190134,597810767,665984147,1415018693,1289310306,940131919,1789083875,1156723497,270737194,1861754121" java.lang.ClassCastException: class zio.FiberRefs cannot be cast to class zio.ZEnvironment (zio.FiberRefs and zio.ZEnvironment are in unnamed module of loader 'app')
[error] at zio.Differ$$anon$5.patch(Differ.scala:146)
[error] at zio.FiberRefPatch.patch(FiberRef.scala:328)
[error] at zio.FiberRefs.$anonfun$forkAs$1(FiberRefs.scala:69)
[error] at scala.collection.immutable.Map$Map4.transform(Map.scala:614)
[error] at scala.collection.immutable.Map$Map4.transform(Map.scala:524)
[error] at zio.FiberRefs.forkAs(FiberRefs.scala:66)
[error] at zio.ZIO$unsafe$.makeChildFiber(ZIO.scala:2603)
[error] at zio.ZIO$unsafe$.fork(ZIO.scala:2587) I didn't have enough time to understand why.
It was too obscure for my small brain to grasp so I tried to understand it and to rewrite it to make it easier for me to understand.
There are 2 sorts of "values" in this code. The initial fiber value ( Feel free to continue and/or copy my work 🙂 |
@guizmaii I'm really sorry to hear about your health issues 😔 I really hope the surgery works and you're back in good shape. Wish you all the best man ✌️🙏 |
@guizmaii oh man, sorry to hear that. I hope the surgery goes well 🙏🙏🙏 |
@guizmaii No rush on this at all! I will close for now, but feel free to re-open if you'd like to push this through. Thank you! |
Combine findings of:
findAncestor
'sparentDepth
parameter usage #8765FiberRefs
implementation guizmaii/zio#2TODO:
findAncestor
. See: [WIP] FixfindAncestor
'sparentDepth
parameter usage #8765 (comment)