4

I was reading through the PerfMark code and saw a comment about avoid an accidental class load through using reflection in a commit:

if (Boolean.getBoolean("io.perfmark.PerfMark.debug")) {
-          Logger.getLogger(PerfMark.class.getName()).log(Level.FINE, "Error during PerfMark.<clinit>", err);
+          // We need to be careful here, as it's easy to accidentally cause a class load.  Logger is loaded
+          // reflectively to avoid accidentally pulling it in.
+          // TODO(carl-mastrangelo): Maybe make this load SLF4J instead?
+          Class<?> logClass = Class.forName("java.util.logging.Logger");
+          Object logger = logClass.getMethod("getLogger", String.class).invoke(null, PerfMark.class.getName());
..
}

I don't quite understand which class is prevented from being accidentally loaded here. According to Class#forName will cause the logger class to be loaded. From my understanding, the class will only be loaded if the enclosing if condition is true. Or is this the point I am missing?

Commit with more context is here: https://github.com/perfmark/perfmark/commit/4f87fb72c2077df6ade958b524d6d217766c9f93#diff-f9fdc8ad347ee9aa7a11a5259d5ab41c81e84c0ff375de17faebe7625cf50fb5R116


I ran the part with the if block and set a breakpoint on static and non-static fields in the Logger class. It hit the breakpoint only when the call was executed irregardless of using reflection or direct. When the if condition was false, no logger was loaded in any case.

Jane
  • 83
  • 6
  • 1
    You are just making your life more difficult. `Logger` will be (class)loaded automatically at the start along with everything else in the JRE. – ControlAltDel Dec 04 '22 at 21:01
  • 2
    I would suspect that the PerfMark code is coded this way to make it compatible with legacy versions of Java that do not include Logger – ControlAltDel Dec 04 '22 at 21:03
  • 1
    @CtrlAltDel your first comment is highly misleading: All classes in the JRE core are __available on the classpath__ on boot. They aren't loaded. JVM bootup would take ages if every class was initialized and loaded that way! - Your second comment is my best guess to. – rzwitserloot Dec 04 '22 at 21:06
  • 1
    @rzwitserloot Sorry it sounded highly misleading to you. What I meant is what you said with "... are available" – ControlAltDel Dec 04 '22 at 21:08
  • 3
    I don't think my interpretation of the word 'loaded' is the central problem here. At any rate, from the scarce comments available (both the code itself, as well as the similarly sparse commit message, which unfortunately does not expand on this nor does it link to an issue with more detail) - I don't think it's possible to tell why this is being done. It certainly isn't to 'avoid the class load', as it wouldn't. I posted a comment on that commit asking for clarification. – rzwitserloot Dec 04 '22 at 21:11

2 Answers2

3

I think the important point of that commit is to load the classes from java.util.logging only when it is really required (when the system property "io.perfmark.PerfMark.debug" is "true" and err is not null, i.e. when the class io.perfmark.impl.SecretPerfMarkImpl$PerfMarkImpl is not available or that class has not the required constructor.)

If the code is

Logger.getLogger(PerfMark.class.getName()).log(Level.FINE, "Error during PerfMark.<clinit>", err);

then the java.util.logging.Logger class may be loaded as soon as the PerfMark class is verified and linked (since linking PerfMark requires that the static initializer block is executed).

With this convoluted code the java.util.logging.Logger is only loaded if PerfMark cannot load its support class io.perfmark.impl.SecretPerfMarkImpl$PerfMarkImpl and the system property "io.perfmark.PerfMark.debug" is set to "true" (which probably means that java.util.logging.Logger is almost never loaded just because you use PerfMark)


The JVM Specification has clauses that loading / verifying / linking of a class is not required to load all the referenced classes, and modern JVM implementations will probably implement many of these points to reduce unnecessary class loading and improve performance. But keep in mind that PerfMark as a very generic library that supports Java versions from 1.6 to the latest versions probably wants to prevent unnecessary class loading even if the JVM does eagerly load referenced classes.

That means that this is a very special technique for a very special library and very special circumstances. If you were to include similar techniques in your code I would object such a change for most places, questioning whether this change is really necessary and supported by rigorous performance tests.

Thomas Kläger
  • 17,754
  • 3
  • 23
  • 34
  • 1
    Are you talking about the execution of `Logger`’s static initializer? That’s still deferred to the first actual invocation of `Logger.getLogger(…)`. The loading of the `Logger` class, on the other hand, is implementation specific. Chances are high that in the non-reflection version, the verifier will cause its loading, regardless of the outcome of the condition. – Holger Dec 22 '22 at 12:16
  • I'm talking solely about loading the class as specified in https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-5.html#jvms-5.3, not about initializing the `java.util.logging.Logger`. Loading of the class is required to verify the code of `PerfMark.` (how else can the class verification check that `Logger.getLogger().log()` is valid?) – Thomas Kläger Dec 25 '22 at 18:14
  • 1
    The exact time of verification is implementation specific. In principle, it is possible to defer the verification of the `Logger.getLogger().log()` statement up to the point when it is actually executed, implying that even the verification might be skipped when the statement is never executed. In practice, it’s more complicated as discussed in [this Q&A](https://stackoverflow.com/q/41965457/2711488). Subtle aspects of the actual code may change the time of verification (and hence, loading of dependencies). – Holger Jan 02 '23 at 11:04
  • 1
    I suggest to change the sentence to “…the `java.util.logging.Logger` class is loaded as soon as the `PerfMark` class is *verified*” which not only is more correct, it also hints at the verifier as the cause of the loading. – Holger Jan 02 '23 at 11:06
1

(I'm the original author of this commit)

There are a few design constraints that libraries are subject to that regular code is not. The purpose of this code is hard to see on the surface, but there are a few goals that this accomplishes.

  • Avoids pulling in the java.logging module. If an application pulls in in PerfMark, but is a modular program, it may wish to avoid including java.logging. Running this code in an IDE, or in a non modular application, hides the issue. Usually these run with the java.se module, which pulls in java.logging. However, when running jlink to produce a reduced size Java image, this code would throw with a NoClassDefError (or similar) due to the class not being on the module path.

    To avoid forcing all applications to take a module dependency on java.logging, the code opts to reflectively load the logger if debugging is desired.

  • To enable adoption of the library, the code needs to be as lightweight as possible. PerfMark will be disabled the majority of the time, so any cost associate with including PerfMark in other libraries should be avoided. I included benchmarks that shows approximately 5ms of speedup during class load.

Hopefully these explain the motivation for the change. If I had known more people were reading the code, I would have included better comments / docs.

Carl Mastrangelo
  • 5,970
  • 1
  • 28
  • 37