-
Notifications
You must be signed in to change notification settings - Fork 529
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
Compute pool metrics #2534
Compute pool metrics #2534
Conversation
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.
Don't have experience with MBeans but this seems straightforward enough 👍
core/jvm/src/main/scala/cats/effect/unsafe/metrics/ComputePoolSampler.scala
Outdated
Show resolved
Hide resolved
private[unsafe] def blockedWorkerThreadCounterForwarder: AtomicInteger = | ||
blockedWorkerThreadCounter |
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.
Pedagogical: why keep this behind a forwarder?
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.
Because of this BS:
class Stupid {
val blah = 5
}
compiles to the following bytecode:
public class cats.effect.Stupid {
private final int blah;
private volatile boolean bitmap$init$0;
public int blah();
Code:
0: aload_0
1: getfield #15 // Field bitmap$init$0:Z
4: ifeq 14
7: aload_0
8: getfield #17 // Field blah:I
11: goto 24
14: new #19 // class scala/UninitializedFieldError
17: dup
18: ldc #21 // String Uninitialized field: /Users/vasil/Code/cats-effect/core/shared/src/main/scala/cats/effect/Stupid.scala: 20
20: invokespecial #25 // Method scala/UninitializedFieldError."<init>":(Ljava/lang/String;)V
23: athrow
24: pop
25: aload_0
26: getfield #17 // Field blah:I
29: ireturn
public cats.effect.Stupid();
Code:
0: aload_0
1: invokespecial #30 // Method java/lang/Object."<init>":()V
4: aload_0
5: iconst_5
6: putfield #17 // Field blah:I
9: aload_0
10: iconst_1
11: putfield #15 // Field bitmap$init$0:Z
14: return
}
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.
The blah()
getter is synchronized on the bitmap
field, which is volatile, incurring unnecessary synchronization points.
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.
Huh, I thought this happened even for private[this]
fields which is why you moved everything to the constructor?
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.
It does in the constructor. But for private[this]
fields, the getter is not automatically generated by the compiler. A "forwarder" method accesses the field raw.
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.
class Forwarder {
private[this] val blah: Int = 5
def blahForwarder: Int = blah
}
compiles to
public class cats.effect.Forwarder {
private final int blah;
private volatile boolean bitmap$init$0;
public int blahForwarder();
Code:
0: aload_0
1: getfield #16 // Field blah:I
4: ireturn
public cats.effect.Forwarder();
Code:
0: aload_0
1: invokespecial #22 // Method java/lang/Object."<init>":()V
4: aload_0
5: iconst_5
6: putfield #16 // Field blah:I
9: aload_0
10: iconst_1
11: putfield #24 // Field bitmap$init$0:Z
14: return
}
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.
Forwarder is a Cats Effect nomenclature. I think I heard Daniel say it once, or use it, and I continued doing so. It doesn't have any meaning. It's the same as getter, the point is to write it ourselves and circumvent the synchronization that the Scala compiler would do otherwise. 😄
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'm very glad there's a workaround! But I wish I understood why the Scala compiler does this.
I've been putting off doing this for so long.