RFR: Instrumentation plugin concept

[Leonid Kuskov]

On Wed, 17 Feb 2021 15:55:56 GMT, Alexandre Iline <shurailine at openjdk.org> wrote:

> These changes are introducing a concept of an instrumentation plugin into JCov. Instrumentation plugin (defined by _com.sun.tdk.jcov.instrument.InstrumentationPlugin_) represents a logic which specifies what code instrumentation is to be performed by JCov. 
> 
> In the current implementation, the concept is only used to define instrumentation **additional** to that which is already done by JCov. The instrumentation performed by such plugins may or may not be relates to the instrumentation already performed by JCov. As such, JCov could be used as only an engine to load and save the code, with the additional instrumentation having nothing to do with code coverage. 
> 
> It is also possible to reuse the same concept **within JCov itself**. Depending on the command line option, JCov already inserts very different instrumentations. This is now controlled programmatically such as in _com.sun.tdk.jcov.instrument.DeferringMethodClassAdapter.visitMethod(int, String, String, String, String[])_ method. Such code could possibly benefit from some delegation. This actual suggested change does not suggest any changes related to code instrumentation currently supported by JCov.
> 
> In addition to the instrumentation plugin, the changes suggest a way to specify a logic to save all data collected during the runtime. That is done by reusing _com.sun.tdk.jcov.runtime.JCovSaver_ interface.
> 
> 
> To better understand the concept perhaps it is helpful to take a look on the test which is suggested as a part of this change: _com.sun.tdk.jcov.instrument.plugin.FieldsTest_ and the accompanying files. The same concept is explained in more details in this draft JEP proposal: https://bugs.openjdk.java.net/browse/JDK-8043291
> 
> 
> More work needed.
> 
> Current state of the code is offered solely for the purpose of discussing the concepts of instrumentation plugin. When the concept is generally accepted, more changes to the source are needed and more testing is needed also. 
> 
> These are only some of the things which needs to be looked into:
> 1. a mechanism is required to notify the plugin about the completion of the instrumentation process. This is needed for the plugin to be able to save data collected during instrumentation, similar to JCov saving a template. To do.
> 2. what is the right way to pass the saver? Currently it is done through JCov properties.
> 3. how to unit test the exit hook? See com.sun.tdk.jcov.runtime.Collect.init(), where there is a hack right now.
> 4. how to test  instrumentation from the same VM? See _com.sun.tdk.jcov.lib.InstrProxy.instr(String[] , String...)_
> 5. allow the instrumentation plugin through all entry points. Right now only _com.sun.tdk.jcov.Instr_ is supported and tested.

There are comments inlined in code review.
Also it would be good to have a working plugin (not just a unit test) that could be used for gathering certain info.

Marked as reviewed by lkuskov (Committer).

build/build.xml line 315:

> 313:             <classpath>
> 314:                 <path path="${testngjar}:${build.dir}/jcov.jar:${result.dir}/test/classes:${jcommander.jar}"/>
> 315:             </classpath>

Actually, build.properties does not contain jcomander.jar property. Please add it.

src/classes/com/sun/tdk/jcov/Instr.java line 658:

> 656:             String pluginClass = opts.getValue(InstrumentationOptions.DSC_INSTR_PLUGIN);
> 657:             if(pluginClass != null && !pluginClass.isEmpty())
> 658:                 plugin = (InstrumentationPlugin) Class.forName(opts.getValue(InstrumentationOptions.DSC_INSTR_PLUGIN)).newInstance();

There should be: forName().getDeclaredConstructor().newInstance()  Class<?>.newInstance() has been deprecated since 9

src/classes/com/sun/tdk/jcov/instrument/InstrumentationOptions.java line 161:

> 159:     public final static OptionDescr DSC_INSTR_PLUGIN =
> 160:             new OptionDescr("instr_plugin", new String[0], "", OptionDescr.VAL_SINGLE,
> 161:                     "TODO");

It's not good idea to put back the empty descriptor for a new option.

src/classes/com/sun/tdk/jcov/instrument/InstrumentationParams.java line 111:

> 109:     }
> 110:     public InstrumentationParams(boolean innerInvocations, boolean classesReload, boolean dynamicCollect, boolean instrumentNative, boolean instrumentFields, boolean detectInternal, ABSTRACTMODE instrumentAbstract, String[] includes, String[] excludes, String[] callerIncludes, String[] callerExcludes, String[] m_includes, String[] m_excludes, InstrumentationMode mode, String[] saveBegin, String[] saveEnd, InstrumentationPlugin plugin) {
> 111: 

It would be good to follow Java Code Conventions: Avoid lines longer than 80 chars.

src/classes/result.xml line 1:

> 1: <?xml version='1.0' encoding='UTF-8'?>

Do we need to have this file in the repository?

src/classes/template.xml line 1:

> 1: <?xml version='1.0' encoding='UTF-8'?>

The file looks superfluous.

-------------

Changes requested by lkuskov (Committer).

PR: https://git.openjdk.java.net/jcov/pull/9