-
Notifications
You must be signed in to change notification settings - Fork 519
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
IOLocal
propagation for unsafe access
#3636
base: series/3.x
Are you sure you want to change the base?
IOLocal
propagation for unsafe access
#3636
Conversation
dc9b74c
to
0b88c01
Compare
@@ -43,4 +43,6 @@ final class IOFiberConstants { | |||
static final byte CedeR = 6; | |||
static final byte AutoCedeR = 7; | |||
static final byte DoneR = 8; | |||
|
|||
static final boolean dumpLocals = Boolean.getBoolean("cats.effect.tracing.dumpLocals"); |
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.
Bikesheddable configuration for opting-in. So the rest of us don't have to pay the penalty 😇
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.
Is this specifically "tracing", even if that's the most obvious use case?
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.
Oh woops, this was a very lazy copy-pasta. I copied it from the system properties we use to configure fiber tracing. We should rename it anyway, dumpLocals
is not quite right I think 😅
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.
What about cats.effect.localContextPropagation
similar to Monix's monix.environment.localContextPropagation
?
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.
Thanks, I liked that! I went with cats.effect.ioLocalPropagation
.
var locals: IOLocals = null | ||
if (dumpLocals) { | ||
locals = new IOLocals(localState) | ||
IOLocals.threadLocal.set(locals) | ||
} |
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 this just for delay
, but I guess blocking
and interruptible
would want it too.
// TODO handle defaults and lenses. all do-able, just needs refactoring ... | ||
final class IOLocals private[effect] (private[this] var state: IOLocalState) { |
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.
Ok, so I was extremely lazy with implementing this thing. But the main idea of this wrapper API is that it should only give the user access to IOLocal
s that they know about i.e. they should not able to clear out other locals that happen to be present.
val fiber = new IOFiber[A]( | ||
Map.empty, | ||
if (IOFiberConstants.dumpLocals) unsafe.IOLocals.getState else Map.empty, |
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.
We can even go in the opposite direction for IO#unsafeRun*
😁
It's less clear if/how to do this for fibers started in a Dispatcher
, since they should be inheriting locals from the fiber backing the Dispatcher
.
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.
Interesting. I will try to give a snapshot of this a shot over on the otel4s side.
@@ -43,4 +43,6 @@ final class IOFiberConstants { | |||
static final byte CedeR = 6; | |||
static final byte AutoCedeR = 7; | |||
static final byte DoneR = 8; | |||
|
|||
static final boolean dumpLocals = Boolean.getBoolean("cats.effect.tracing.dumpLocals"); |
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.
Is this specifically "tracing", even if that's the most obvious use case?
// defined in Java since Scala doesn't let us define static fields | ||
final class IOFiberConstants { | ||
public final class IOFiberConstants { |
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.
This is not good. To avoid this we'll either have to replicate it at both the cats.effect
and cats.effect.unsafe
levels, or move the thread-local IOLocals
accessors into cats.effect
.
I apologise for jumping in and asking this question but, can it be used as an alternative to Local from Monix? |
@kevin-lee no problem! At a glance, yes, this does look like an alternative/replacement to that Monix feature. Perhaps @alexandru can confirm :) |
IOLocal
sIOLocal
s
@armanbilge Thank you. That's great! |
@kevin-lee the For an example integration, see this PR which implements a Java SPI using this |
@armanbilge Oh... got it. It looks very promising then. Thank you! |
@armanbilge what's the status of this? |
I intend to land this in the upcoming v3.6.0 release (no ETA for this yet, but I'll make a milestone). The benchmarks I posted above for the initial implementation were disappointing, but I still need to benchmark the new implementation that I pushed in 3589db4. |
@armanbilge That's great. Thank you! I'm sure you can optimize it afterwards, and in the meantime, it would be extremely useful for people who need this feature. |
Unfortunately it's not so simple. The performance impact I reported before was for all users of Cats Effect, including those who are not using this feature. We want to avoid slowing down the entire runtime for everyone, because that would be a serious regression. So before we can release this, we need to minimize its impact on existing users who do not need this feature. Still I'm optimistic about the performance of the revised implementation. Will report updated benchmarks, soon ... |
How'd it go @armanbilge ? If you've had time |
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 think I've come round to the opinion this is a necessary evil, it's just bikeshedding some of the API and vetting out a few concerns about the implementation.
if (ioLocalPropagation) { | ||
IOFiber.setCurrentIOFiber(null) | ||
} |
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.
We definitely need to do some careful benchmarking here. This particular bit is very sensitive to inliner shenanigans.
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.
We meaning you 😛
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.
Yeah yeah yeah… Give me a pair of shas! :)
@@ -46,6 +46,8 @@ private object IOFiberConstants { | |||
final val AutoCedeR = 7 | |||
final val DoneR = 8 | |||
|
|||
final val ioLocalPropagation = false |
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.
Since JS/Native currently lack a mechanism to eliminate branches based on system properties at link- or run-time we just hard-code to false
to DCE at compile-time.
/** | ||
* Returns a [[java.lang.ThreadLocal]] view of this [[IOLocal]] that allows to unsafely get, | ||
* set, and remove (aka reset) the value in the currently running fiber. The system property | ||
* `cats.effect.ioLocalPropagation` must be `true`, otherwise throws an | ||
* [[java.lang.UnsupportedOperationException]]. | ||
*/ | ||
def unsafeThreadLocal(): ThreadLocal[A] = if (ioLocalPropagation) |
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.
This is the new API based on the ThreadLocal
interface. Thoughts?
-
I made it JVM-only. We could definitely support this on JS/Native but I'm not yet convinced that there is a good reason to do so: no Java libs to interop with and performance implications.
-
If
ioLocalPropagation == false
it throws anUnsupportedOperationException
. Libraries can checkIOLocal.isPropagating
before deciding to call this method.Alternatively we could return a no-op
ThreadLocal
that never sets and always returns the default value. -
If we decide not to throw, we could make this a
lazy val
(or similar) instead. Creating theThreadLocal
"view" isn't observably effectual.
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 think this is great, all of it.
Finished benchmarking. @armanbilge let's see your armchair performance reasoning explain this result. I'm at a loss. Before
After
|
Finally scraped together the time needed to shave a few dev environment yaks and test this PR with propagation enabled. The performance hit is about 25% on microbenchmarks involving So I think the summary of this PR, from a performance standpoint, is the following:
So enabling this is analogous to enabling cached tracing, but of course would be cumulative with that flag as well, so technically turning both of them on would bop your microbenchmark scores by something like 55%. It's very unclear whether this type of performance impact matters in practice. We would probably need to run some end-to-end scale tests with propagation enabled in order to see. Given that this is default-off and the impact when disabled is zero (outside of the still bizarre WSTP microbenchmark mirages), I'm comfortable merging this as-is and we can do some further testing. We should also probably document (or at least, write a ticket to document) the performance effects so it doesn't surprise anyone. |
I added some documentation.
Discussed on Discord, but to clarify there is no reason to expect that |
@djspiewak Please forgive my impetuousness - do you have a guesstimate of when this might be going in please? |
Still needs quite a bit of work, but wanted to sketch the basic idea.
Goal: to expose a fiber's
IOLocal
s asThreadLocal
s within a side-effecting block, so that they can be accessed and modified in unsafe land.Motivation: basically telemetry Java interop.
Constraints: to do this as safely as possible 😅