Skip to content

fix #3648 added Ability to set DebugProbes.enableCreationStackTraces using system property #3764

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix #3648 added Ability to set DebugProbes.enableCreationStackTraces using system property #3764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions kotlinx-coroutines-core/jvm/src/debug/AgentPremain.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,11 @@ import java.security.*
@IgnoreJRERequirement // Never touched on Android
internal object AgentPremain {

private val enableCreationStackTraces = runCatching {
System.getProperty("kotlinx.coroutines.debug.enable.creation.stack.trace")?.toBoolean()
}.getOrNull() ?: DebugProbesImpl.enableCreationStackTraces

@JvmStatic
@Suppress("UNUSED_PARAMETER")
fun premain(args: String?, instrumentation: Instrumentation) {
AgentInstallationType.isInstalledStatically = true
instrumentation.addTransformer(DebugProbesTransformer)
DebugProbesImpl.enableCreationStackTraces = enableCreationStackTraces
DebugProbesImpl.install()
installSignalHandler()
}
Expand Down
4 changes: 3 additions & 1 deletion kotlinx-coroutines-core/jvm/src/debug/internal/DebugProbesImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ internal object DebugProbesImpl {
private val sequenceNumber = atomic(0L)

public var sanitizeStackTraces: Boolean = true
public var enableCreationStackTraces: Boolean = true
public var enableCreationStackTraces: Boolean = runCatching {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dkhalanskyjb WDYT about adding system property only for the enableCreationStackTraces here with an extra comment?

My initial line of thought was that creation stacktraces are the only imporant/performance-sensitive part that folks who run agent externally might want to disable explicitly via system property.
Yet indeed it makes sense to work both ways

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand the alternative you're implying here. Do you propose adding a system property to sanitizeStackTraces as well? If so, here are my preferences, from most desirable to least desirable:

  • Don't agree to the proposal at all. If someone is calling DebugProbes.install, I don't see at all why it's a big deal also to write enableCreationStackTraces = false. When used as an agent, there are no other sensible entry points where this parameter can be placed, so a system property makes sense, but here, when you have DebugProbes.install anyway? Either I'm missing something, or this is code golfing, and the cost is that the way you run your code may start affecting what happens in non-obvious ways.
  • Only add enable.creation.stack.trace. Even if, for some reason, we have to add it, it's an unpleasant workaround around some issue that I don't understand, not a valid API entry point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You got it indeed; I was unsure about the change (it seemed rather ad-hoc), and the rationale "We prefer system properties over programmatic API" was not sufficient to me, wanted to have a second opionion

Copy link

Choose a reason for hiding this comment

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

but here, when you have DebugProbes.install anyway

In our classloader, that can return a correct class implementation. That's why we cannot call enableCreationStackTraces = false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is a "correct class implementation"? Also, install doesn't return anything.

Copy link

Choose a reason for hiding this comment

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

For what I need — to get rid of byte-buddy and avoid performance regression https://youtrack.jetbrains.com/issue/IDEA-313677/Performance-degradation-in-findUsages-localInspections-and-completion

    if (packageNameHash == -3930079881136890558L &&
        enableCoroutineDump &&
        className.equals("kotlin.coroutines.jvm.internal.DebugProbesKt")) {
      String resourceName = "DebugProbesKt.bin";
      Resource resource = findResource(resourceName);
      if (resource == null) {
        //noinspection UseOfSystemOutOrSystemErr
        System.err.println("Cannot find " + resourceName);
      }
      else {
        return classDataConsumer.consumeClassData(className, resource.getByteBuffer());
      }
    }
Screenshot 2023-06-28 at 14 32 24

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is consumeClassData something you use to emulate the effect of install()? If so, is it impossible to call enableCreationStackTraces = false before returning the result?

Copy link

Choose a reason for hiding this comment

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

is it impossible to call

No. To call it, we have to load (defineClass) some class from coroutine lib, and we cannot do it. That's why I ask system property.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've reopened #3648

to continue the discussion and/or explain why it's important. The original report hasn't stated all the intentions and limitations, thus was hard to reason about

System.getProperty("kotlinx.coroutines.debug.enable.creation.stack.trace")?.toBoolean()
}.getOrNull() ?: true

/*
* Substitute for service loader, DI between core and debug modules.
Expand Down