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

[PavlovIvan]

No description provided.

[qwwdfsad]

@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

[dkhalanskyjb]

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.

[qwwdfsad]

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

[develar]

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.

[dkhalanskyjb]

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

[develar]

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());
      }
    }

[dkhalanskyjb]

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

[develar]

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.

[qwwdfsad]

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

[qwwdfsad]

Closing as won't fix

[dkhalanskyjb]

I, for one, don't need to be convinced any further, I'm okay with just merging this. I don't understand the mechanism used as an alternative for the agent, yet since it's a non-programmatic invocation that happens externally to the observed program, the same reasoning as we have for the agent fully applies. So, it is a workaround for a highly-specific scenario.