RFR: 8294982: Implementation of Classfile API [v20]

Thu Feb 16 15:03:58 UTC 2023

On Thu, 16 Feb 2023 10:41:33 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> This is root pull request with Classfile API implementation, tests and benchmarks initial drop into JDK.
>> 
>> Following pull requests consolidating JDK class files parsing, generating, and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to this one.
>> 
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>> 
>> Development branch of consolidated JDK class files parsing, generating, and transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>> 
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online API documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) is also available.
>> 
>> Please take you time to review this non-trivial JDK addition.
>> 
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional commit since the last revision:
> 
>   added 4-byte Unicode text to Utf8EntryTest

src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java line 75:

> 73:     static ConstantPoolBuilder of(ClassModel classModel) {
> 74:         ClassReader reader = (ClassReader) classModel.constantPool();
> 75:         return reader.optionValue(Classfile.Option.Key.CP_SHARING)

Question: the fact that, when transforming, the new constant pool is the same as the old one, plus some "appended" entries at the end, is a pretty crucial property to ensure that existing code that is not transformed (e.g. unknown attributes) remains legal after transformation. But it seems that, if the `CP_SHARING` option is not passed, we build a brand new constant pool, in which case there's no guarantee that old indices will still point to the same position. Doesn't that lead to issue with unknown attributes?

I guess my question is: shouldn't the semantics properties/guarantees of a classfile transform be specified regardless of the options being passed in? E.g. it's ok if using different options ends up with different performance model, but I'm a bit worried if options can affect correctness of the transform?

src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java line 38:

> 36:     private final List<Attribute<?>> attributes = new ArrayList<>();
> 37: 
> 38:     public AttributeHolder() {

default constructor

src/java.base/share/classes/jdk/internal/classfile/impl/BytecodeHelpers.java line 54:

> 52:  */
> 53: public class BytecodeHelpers {
> 54: //    public static Map<ConstantDesc, Opcode> constantsToOpcodes = new HashMap<>(16);

Should this be removed?

src/java.base/share/classes/jdk/internal/classfile/impl/BytecodeHelpers.java line 56:

> 54: //    public static Map<ConstantDesc, Opcode> constantsToOpcodes = new HashMap<>(16);
> 55: 
> 56:     public BytecodeHelpers() {

Should this also be removed (same as default constructor) ?

src/java.base/share/classes/jdk/internal/classfile/impl/ChainedFieldBuilder.java line 48:

> 46:         this.consumer = consumer;
> 47:         FieldBuilder b = downstream;
> 48:         while (b instanceof ChainedFieldBuilder cb)

I'm probably missing something - but if `b` is another chained builder, instead of using a loop, can't we just skip to its  `terminal` field? Also, the `downstream` field seems unused. (same considerations apply for `ChainedMethodBuilder` and `ChainedClassBuilder`).

I note that `NonTerminalCodeBuilder` seems to be already doing what I suggest here (e.g. skip to `terminal`).

src/java.base/share/classes/jdk/internal/classfile/impl/ConcreteEntry.java line 58:

> 56: import jdk.internal.classfile.jdktypes.PackageDesc;
> 57: 
> 58: public abstract sealed class ConcreteEntry {

Why the name `concrete` ? Am I missing something (e.g. existence of "non-concrete" pool entries?)

src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java line 649:

> 647:             if (parentMap == null)
> 648:                 parentMap = new IdentityHashMap<>();
> 649:             int[] table = parentMap.computeIfAbsent(parent, new Function<CodeAttribute, int[]>() {

Can use a lambda here?

src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java line 658:

> 656:             mruParent = parent;
> 657:             mruParentTable = table;
> 658:             return mruParentTable[lab.getContextInfo()] - 1;

Am I correct that this code can misbehave e.g. if `computeIfAbsent` ends up creating a brand new `table` array - in which case, all array elements are set to `0` - meaning we end up returning `-1`. Is that what we want?

src/java.base/share/classes/jdk/internal/classfile/impl/InstructionData.java line 47:

> 45:  * InstructionData
> 46:  */
> 47: public class InstructionData {

As CodeImpl seems to be the only client of this, I wonder if we could move the static cache in there?

src/java.base/share/classes/jdk/internal/classfile/impl/LabelImpl.java line 65:

> 63: 
> 64:     public int getContextInfo() {
> 65:         return contextInfo;

This seems to be the BCI - should we change names to reflect that? (`contextInfo` seems very vague)

src/java.base/share/classes/jdk/internal/classfile/impl/TransformImpl.java line 51:

> 49:  * TransformImpl
> 50:  */
> 51: public class TransformImpl {

Understanding check: a transform implementation is made up of two runnable (start/end handlers) and a consumer. The consumer can be used e.g. to iterate and transform all the contents of a compound element (e.g. all the instruction in a code attribute).

User-defined transforms can be "resolved" that is, we can "bind" them to a specific builder, which effectively turns them into consumers (and into a concrete transform implementation that can be used).

When two transforms T1 and T2 are composed (so that T1 runs before T2), we need to implement the `resolve` method of the resulting transform so that:
* We resolve T2 against the provided builder - this gives us a "downstream" consumer;
* We create a new "chained" builder which wraps the provided builder, and whose `with` method calls the downstream consumer;
* We then resolve T1 against the chained builder, and return the resulting consumer

This means that when we call `accept` on the resulting consumer, we will transform (using T1) and call `with` (passing the transformed element)  on the chained builder, which will end up calling `accept` on the downstream consumer, which will apply T2 and finally add the resulting element to the provided builder.

Correct? (Phew)

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

PR: https://git.openjdk.org/jdk/pull/10982