Optimize ZIO#provideSomeEnvironment and FiberRef#locallyWith
#9652
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guizmaii
reviewed
Feb 27, 2025
| @@ -467,54 +467,73 @@ object FiberRef { | |||
| self => | |||
| type Patch = Patch0 | |||
|
|
|||
| def combine(first: Patch, second: Patch): Patch = | |||
| final def combine(first: Patch, second: Patch): Patch = | |||
There was a problem hiding this comment.
What about using a final class here instead of an anonymous one? 🤔
Anonymous classes generates quite ugly stack traces
final class PatchFiber[Value0, Patch0] extends FiberRef[Value0] {
type Patch = Patch0
...
}There was a problem hiding this comment.
I thought about that too, I'll change it 👍
hearnadam
previously approved these changes
Feb 28, 2025
| ZIO.uninterruptibleMask { restore => | ||
| restore(zio).exitWith { exit => | ||
| val oldValue = oldRefs.getOrNull(self) | ||
| if (oldValue == null) fiberState.resetFiberRef(self) |
There was a problem hiding this comment.
Suggested change
| if (oldValue == null) fiberState.resetFiberRef(self) | |
| if (oldValue eq null) fiberState.resetFiberRef(self) |
There was a problem hiding this comment.
oldValue is not an AnyRef so we can't use eq
Comment on lines
+1290
to
+1291
| FiberRef.currentEnvironment | ||
| .locallyWith(f.asInstanceOf[ZEnvironment[Any] => ZEnvironment[Any]])(self.asInstanceOf[ZIO[Any, E, A]]) |
There was a problem hiding this comment.
Why is the casting necessary?
There was a problem hiding this comment.
Because FiberRef.currentEnvironment contains a ZEnvironment[Any] (which is not very correct I know but it's a package-private val) so we have to cast the function to ZEvironment[Any] => ZEvironment[Any] for the code to compile
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ZIO#provideSomeEnvironmentis one of the methods that can be commonly evaluated (depending on the workflow) since it's used byZIO.scoped. Currently it requires an invocation ofFiberRef#getand another ofFiberRef#locally. We can "optimize" it by usingFiberRef#locallyWith.However, since
locallyWithis actually implemented asgetWith+locallythere isn't much benefit of simply changing it. This PR also optimizeslocallyWithby making it "unfinal" and overriding its implementation to avoid the additionalFiberRef#getWith.I also made some other minor optimizations such as making the methods in the anonymous class final (since for some reason the Scala compiler doesn't make anonymous classes final 🙄)