Revisit j.l.classfile.CodeBuilder API surface

[Adam Sotona]

>From JDK perspective I see no problem to drop operator(opcode).

More of an expectation than a surprise is frequent use of constantInstruction, so refactored a lot of cases.
One surprise is heavy use of xyzInstruction methods in our own tests (originated from the times before “conveniences” appear). However, these are worth of update anyway.

From: Brian Goetz <brian.goetz at oracle.com>
Date: Friday, 5 January 2024 at 16:52
To: Adam Sotona <adam.sotona at oracle.com>, classfile-api-dev at openjdk.org <classfile-api-dev at openjdk.org>
Subject: Re: Revisit j.l.classfile.CodeBuilder API surface
Let me fill in some history here.

Much of the design process for this library could be described as an iterated game of "find the primitive."  The first version of the API had a zillion ad-hoc methods, with little structural relation.  At some point we hit upon a model-driven analysis, and broke down instructions into families (loads, stores, etc), and at that point, we thought methods like loadInstruction were the primitives.  But we hadn't actually hit bottom yet; after many rounds of refactoring, it became clear that there was one primitive for the builder hierarchy, now called `with`, and all of the builder methods now bottom out at that.

At the same time, we added a number of "convenience" methods such as aload_0() and aload(int n) (which now just delegate to with() or loadInstruction()), and over time, we found that most generative use cases used these methods much more than the primitives (which is fine.)

At this point, I think many of the xxxInstruction (and its rigid naming convention) are vestiges of a previous attempt to organize the API.  Some still have uses, though should be renamed to reflect that they are mere conveniences (such as arrayLoadInstruction to arrayLoad).

(Does anyone use operator(opcode), or can we drop that one too?)

So I support this simplification.

Having done the refactor, were there any surprises in the usages of various CodeBuilder methods?

On 1/5/2024 10:38 AM, Adam Sotona wrote:
Hi,
j.l.classfile.CodeBuilder API consist of more than 230 methods.
Existing ClassFile API use cases proved the concept of one big CodeBuilder is comfortable. However there are some redundancies, glitches in the naming, some frequently used methods are hard to find and some methods have low practical use.

Majority of the methods may be divided into three main levels:
1. methods building low level bytecode instructions according to JVMS chapter 6.5 (aaload, aastore, aconst_null...)
2. methods reconstructing individual subtypes of j.l.classfile.Instruction from given arguments (loadInstruction, storeInstruction, incrementInstruction, branchInstruction...)
3. methods modeling high level code blocks (block, ifThen, ifThenElse, trying...)

Many methods from level 2 (with suffix `Instruction`) seem to be obsolete or misplaced. Some of them are duplicates of methods from level 1, some are obsolete and some are very useful, however a bit hidden. The API should be cleaned a bit while in preview.

I would like to open a discussion on the following proposed changes in the CodeBuilder methods:

  *   incrementInstruction remove as duplicate of iinc
  *   lookupSwitchInstruction remove as duplicate of lookupswitch
  *   tableSwitchInstruction remove as duplicate of tableswitch
  *   throwInstruction remove as duplicate of athrow
  *   invokeDynamicInstruction remove as duplicate of invokedynamic
  *   stackInstruction remove as obsolete with suggested replacements:  with(StackInstruction.of(...))
  *   monitorInstruction remove as obsolete with suggested replacements: monitorenter, monitorexit, or with(MonitorInstruction.of(...))
  *   nopInstruction remove as duplicate of nop
  *   typecheckInstruction remove as obsolete with suggested replacements: checkcast, instanceOf, or with(TypeCheckInstruction.of(...))
  *   loadInstruction rename to loadLocal
  *   storeInstruction rename to storeLocal
  *   branchInstruction rename to branch
  *   invokeInstruction rename to invoke
  *   newObjectInstruction rename to newObject
  *   newPrimitiveArrayInstruction rename to newPrimitiveArray
  *   newReferenceArrayInstruction rename to newReferenceArray
  *   newMultidimensionalArrayInstruction rename to newMultidimensionalArray
  *   arrayLoadInstruction rename to arrayLoad
  *   arrayStoreInstruction rename to arrayStore
  *   convertInstruction rename to conversion
  *   operatorInstruction rename to operator
  *   constantInstruction rename to loadConstant
  *   fieldInstruction rename to fieldAccess
  *   instanceof_ rename to instanceOf
  *   returnInstruction rename to return_
Here is the related RFE: https://bugs.openjdk.org/browse/JDK-8323058
Draft of the CSR (no spec yet): https://bugs.openjdk.org/browse/JDK-8323067
And draft of the Pull Request: https://github.com/openjdk/jdk/pull/17282

Any comments are welcome.

Thank you,
Adam

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20240105/e3804001/attachment-0001.htm>