On Thu, 16 Feb 2023 10:41:33 GMT, Adam Sotona <asot...@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
