JDK-8339256 : Optimize ClassFile API loadConstant
  • Type: CSR
  • Component: core-libs
  • Sub-Component: java.lang.classfile
  • Priority: P4
  • Status: Closed
  • Resolution: Approved
  • Fix Versions: 24
  • Submitted: 2024-08-29
  • Updated: 2024-09-20
  • Resolved: 2024-09-20
Related Reports
CSR :  
Relates :  
Description
Summary
-------

Add `int`, `long`, `float`, `double` overloads to `CodeBuilder::loadConstant`.

Problem
-------

Currently `loadConstant` goes through a megamorphic call site of `ConstantDesc`. If users are just passing primitive numbers, they have to go through redundant boxing and pattern matching. In addition, if the user messed up the type of `ConstantDesc`, they may have wrong types pushed to the bytecode stack.

Solution
--------

Provide primitive overloads to `loadConstant`. This avoids boxing for those cases, and the instruction optimization checks in those overloads are simpler. By specifying the type at the bytecode generator call site, we can ensure we push the right constants in the generated code, such as `loadConstant(1)` vs `loadConstant(1L)`.

Specification
-------------

```
    /**
     * Generate an instruction pushing a constant int value onto the operand stack.
     * This is equivalent to {@link #loadConstant(ConstantDesc) loadConstant(Integer.valueOf(value))}.
     * @param value the int value
     * @return this builder
     * @since 24
     */
    default CodeBuilder loadConstant(int value) {

    /**
     * Generate an instruction pushing a constant long value onto the operand stack.
     * This is equivalent to {@link #loadConstant(ConstantDesc) loadConstant(Long.valueOf(value))}.
     * @param value the long value
     * @return this builder
     * @since 24
     */
    default CodeBuilder loadConstant(long value) {

    /**
     * Generate an instruction pushing a constant float value onto the operand stack.
     * This is equivalent to {@link #loadConstant(ConstantDesc) loadConstant(Float.valueOf(value))}.
     * @param value the float value
     * @return this builder
     * @since 24
     */
    default CodeBuilder loadConstant(float value) {

    /**
     * Generate an instruction pushing a constant double value onto the operand stack.
     * This is equivalent to {@link #loadConstant(ConstantDesc) loadConstant(Double.valueOf(value))}.
     * @param value the double value
     * @return this builder
     * @since 24
     */
    default CodeBuilder loadConstant(double value) {
```

Comments
Moving to Approved using "equivalent" rather than "identical."
20-09-2024

A close analogy of this specification can be found in `Field::setBoolean`: ``` * Sets the value of a field as a {@code boolean} on the specified object. * This method is equivalent to * {@code set(obj, zObj)}, * where {@code zObj} is a {@code Boolean} object and * {@code zObj.booleanValue() == z}. ``` Both API tries to assure users that calls with or without boxing will have the same effect. Since this is caller-oriented specification, it should belong to API specs, and the text as-is should suffice. Moving this back to finalized.
17-09-2024

I would like [~kbourril] to weigh in and decide if this information belongs to API/implementation specification/notes; Kevin noted that this information is crucial to avoid confusions around these box-avoiding overloads. One solution I think of is to keep this "identical to"/"equivalent to" information still part of the API spec as it's oriented toward users; the API notes can say something like "Behavior is same for boxed and unboxed overloads to avoid confusions".
17-09-2024

[~liach], yes, the canonical use of an `@implSpec` tag is on a default method in an interface that can generally be extended (i.e. not a `sealed` interface, etc.). There are various example along the lines of "This method is equivalent to ..." in java.base. Perhaps the wording from one of those methods could be useful here.
17-09-2024

[~darcy] From my knowledge, `@implSpec` tags are mostly used to specify the implementation of the immediate method. These new methods conform to this piece of specification by having the `loadConstant(ConstantDesc)` delegate to these primitive versions when the arguments are wrappers, instead of calling `loadConstant(Integer.valueOf(value))` etc. themselves. Is this still considered a good use of `@implSpec`? More background information: This sentence is to assure users that calling this method with or without boxing will have the same outcome to avoid error-prone scenarios, instead of restricting the implementations.
17-09-2024

Moving to Provisional, not Approved. [~liach], while give the other constraints on `CodeBuilder`, it is not strictly required for the methods to have `@implSpec` tags, the " This is identical to..." sentences would work as `@implSpec` tags.
17-09-2024

Proposed for a first round of review: there was concerns about too many overloads, but the number of overloads is limited (just 5 computational types, 4 primitive overloads), and this is the only method in `CodeBuilder` that needs overloads. This should be helpful for improving bootstrap performance. Requesting [~redestad] for a review too.
14-09-2024