Backport fix made in #8765, improve readibility and optimize `FiberRefs` stack implementation #8779

guizmaii · 2024-04-22T08:41:07Z

Combine findings of:

TODO:

Add tests to codify behavior of findAncestor. See: [WIP] Fix findAncestor's parentDepth parameter usage #8765 (comment)

kyri-petrou

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!

kyri-petrou · 2024-04-23T07:04:47Z

core/shared/src/main/scala/zio/FiberRef.scala


-        def combine(first: Patch, second: Patch): Patch =
+        override final def combine(first: Patch, second: Patch): Patch =


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

Personally I find reading stack traces & profiling data a lot more annoying with anonymous classes

Completely agree :)

kyri-petrou · 2024-04-23T07:11:05Z

core/shared/src/main/scala/zio/FiberRefs.scala

@@ -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[_]]


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

Let's try it. Thanks :)

kyri-petrou · 2024-04-23T07:23:30Z

core/shared/src/main/scala/zio/FiberRefs.scala

@@ -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 = {


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))

core/shared/src/main/scala/zio/FiberRefs.scala

kyri-petrou · 2024-04-23T07:44:56Z

core/shared/src/main/scala/zio/FiberRefs.scala

+    /**
+     * Add a new entry on top of the Stack
+     */
+    @inline def put(fiberId: FiberId.Runtime, value: Any): FiberRefStack[?] =


Suggested change

@inline def put(fiberId: FiberId.Runtime, value: Any): FiberRefStack[?] =

@inline def put(fiberId: FiberId.Runtime, value: A): FiberRefStack[A] =

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 🤔

My bad. Found how:

fiberRefStack.asInstanceOf[FiberRefs.FiberRefStack[fiberRef.Value]].put(childId, newValue)

core/shared/src/main/scala/zio/FiberRefs.scala

…rRefs` stack implementation

Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>

guizmaii · 2024-04-26T06:20:19Z

.vscode/settings.json

@@ -1,11 +0,0 @@
-{


TODO @guizmaii: Put back

kyri-petrou · 2024-04-26T07:47:36Z

core/shared/src/main/scala/zio/FiberRefs.scala

-          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


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

ghostdogpr · 2024-04-27T00:21:40Z

@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?

kyri-petrou · 2024-04-29T23:27:36Z

@guizmaii let me know once you'd like me to review again

guizmaii · 2024-05-03T10:55:25Z

@ghostdogpr

Quick answer:

do I wait for this to be merged?

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 😕
I have surgery planned for the 13th of May to fix that.
So I'll probably not be able to spend the necessary time on this before some time 😕

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:

The first thing I was exploring was the one explained here: Backport fix made in #8765, improve readibility and optimize FiberRefs stack implementation #8779 (comment)

My plan was to write tests for the #joinAs function to explore and specify the behaviour of the findAncestor inner function.
But right now, I have introduced a ClassCastException: class zio.FiberRefs cannot be cast to class zio.ZEnvironment issue with my changes 🤔
We can see it when running the test in FiberRefsSpec (you can run them with sbt coreTestsJVM/testOnly *.FiberRefsSpec):

[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.

The second thing I was exploring was initiated by the fact that I didn't understand the code

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.
This work is mainly the change of the Value case class to a specialised data structure (FiberRefStack, and its associated methods/functions).
IMO, it makes the code much more explicit about what it's doing.
To give an example of what I think is a readability improvement, you can compare these 3 lines: https://github.com/zio/zio/pull/8779/files#diff-672fc2acd16d75c9b38db294811088d1f64bf8f994a05d7ef31c610e81c8ce1eR169-R171
with the corresponding ones here:
https://github.com/zio/zio/pull/8779/files#diff-672fc2acd16d75c9b38db294811088d1f64bf8f994a05d7ef31c610e81c8ce1eL167-L173

The third thing I was planning to explore is related to the fact that we're constantly comparing "values"

There are 2 sorts of "values" in this code. The initial fiber value (FiberRef#initial) and the stack "values", each stack layer containing, amongst over things, a value.
If I understand things correctly, it seems that the first value of the stack is the same as the initial fiber value.
Therefor, to know if we have an empty stack, we need to compare the FiberRef#initial with the stack head value.
See https://github.com/zio/zio/pull/8779/files#diff-672fc2acd16d75c9b38db294811088d1f64bf8f994a05d7ef31c610e81c8ce1eR148-R149
This could be quite an inefficient mechanism based on what we compare and was planning, later, in another PR, to see if we could find another way to represent things so that we don't need to have these comparisons and to see where it would lead.

Feel free to continue and/or copy my work 🙂
I'll try my best to continue to work on this; particularly on the point 1, the point 2 being IMO addressed and the point 3 being for later; but I can't promised any ETA

kyri-petrou · 2024-05-03T14:21:56Z

@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 ✌️🙏

ghostdogpr · 2024-05-03T22:44:27Z

@guizmaii oh man, sorry to hear that. I hope the surgery goes well 🙏🙏🙏

This was referenced Apr 22, 2024

[WIP] Improve code readability #8766

Closed

[WIP] Fix findAncestor's parentDepth parameter usage #8765

Closed

guizmaii force-pushed the improve_readability branch from afff585 to 46576b5 Compare April 23, 2024 06:36

kyri-petrou reviewed Apr 23, 2024

View reviewed changes

guizmaii and others added 14 commits April 26, 2024 14:04

Make Patch.Empty a case object

b5a8e0e

Add missing final

e602bf6

Clean code

d2bbb0b

Add missing final

f859fae

Backport fix made in zio#8765, improve readibility and optimize `Fibe…

dc2cee0

…rRefs` stack implementation

Add missing keywords

c3e2d80

Update core/shared/src/main/scala/zio/FiberRefs.scala

3287898

Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>

Update core/shared/src/main/scala/zio/FiberRefs.scala

f68d61c

Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>

Update core/shared/src/main/scala/zio/FiberRefs.scala

e687dd8

Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>

Update core/shared/src/main/scala/zio/FiberRefs.scala

bd19e6f

Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>

Update core/shared/src/main/scala/zio/FiberRefs.scala

69bcf69

Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>

Update core/shared/src/main/scala/zio/FiberRefs.scala

78eaaf0

Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>

Update core/shared/src/main/scala/zio/FiberRefs.scala

8baa618

Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>

Fix compilation error

ac0f395

guizmaii force-pushed the improve_readability branch from 163a49e to ac0f395 Compare April 26, 2024 06:04

guizmaii added 3 commits April 26, 2024 14:14

Implement @kyri-petrou FiberRefPatch idea

eefdedc

Rollback some changes

2fb2f67

clean

ddd86c0

guizmaii commented Apr 26, 2024

View reviewed changes

.vscode/settings.json

@@ -1,11 +0,0 @@

{

Copy link

Member Author

guizmaii Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO @guizmaii: Put back

guizmaii added 2 commits April 26, 2024 14:27

Fix bin compat

6c69a57

Init FiberRefs#joinAs spec

d981436

kyri-petrou reviewed Apr 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport fix made in #8765, improve readibility and optimize `FiberRefs` stack implementation #8779

Backport fix made in #8765, improve readibility and optimize `FiberRefs` stack implementation #8779

guizmaii commented Apr 22, 2024

kyri-petrou left a comment

kyri-petrou Apr 23, 2024

guizmaii Apr 26, 2024

kyri-petrou Apr 23, 2024

guizmaii Apr 26, 2024

kyri-petrou Apr 23, 2024 •

edited

kyri-petrou Apr 23, 2024

guizmaii Apr 26, 2024

guizmaii Apr 26, 2024

guizmaii Apr 26, 2024

kyri-petrou Apr 26, 2024 •

edited

ghostdogpr commented Apr 27, 2024

kyri-petrou commented Apr 29, 2024

guizmaii commented May 3, 2024 •

edited

kyri-petrou commented May 3, 2024

ghostdogpr commented May 3, 2024


		def combine(first: Patch, second: Patch): Patch =
		override final def combine(first: Patch, second: Patch): Patch =

	@inline def put(fiberId: FiberId.Runtime, value: Any): FiberRefStack[?] =
	@inline def put(fiberId: FiberId.Runtime, value: A): FiberRefStack[A] =

Backport fix made in #8765, improve readibility and optimize FiberRefs stack implementation #8779

Are you sure you want to change the base?

Backport fix made in #8765, improve readibility and optimize FiberRefs stack implementation #8779

Conversation

guizmaii commented Apr 22, 2024

kyri-petrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyri-petrou Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyri-petrou Apr 26, 2024 • edited

Choose a reason for hiding this comment

ghostdogpr commented Apr 27, 2024

kyri-petrou commented Apr 29, 2024

guizmaii commented May 3, 2024 • edited

Quick answer:

Long answer:

kyri-petrou commented May 3, 2024

ghostdogpr commented May 3, 2024

Backport fix made in #8765, improve readibility and optimize `FiberRefs` stack implementation #8779

Backport fix made in #8765, improve readibility and optimize `FiberRefs` stack implementation #8779

kyri-petrou Apr 23, 2024 •

edited

kyri-petrou Apr 26, 2024 •

edited

guizmaii commented May 3, 2024 •

edited