Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records #3201

Closed
wants to merge 13 commits into from

Conversation

@dfuch
Copy link
Member

@dfuch dfuch commented Mar 25, 2021

This RFE proposes to extend the MXBean framework to define a mapping to records.

The MXBean framework already defines a mapping of CompositeType to plain java objects. Records are a natural representation of CompositeTypes. A record can be easily reconstructed from a CompositeData through the record canonical constructor. A clear advantage of records over plain java objects is that the canonical constructor will not need to be annotated in order to map composite data property names to constructor parameter names.

With this RFE, here is an example comparing coding a composite type NamedNumber that consists of an int and a String, using records and using a plain java class. In both case, the CompositeType looks like this:

CompositeType(
    "NamedNumber",                      // typeName
    "NamedNumber",                      // description
    new String[] {"number", "name"},    // itemNames
    new String[] {"number", "name"},    // itemDescriptions
    new OpenType[] {SimpleType.INTEGER,
                    SimpleType.STRING}  // itemTypes
);

The plain Java class needs a public constructor annotated with @ConstructorParameters annotation:

public class NamedNumber {
    public int getNumber() {return number;}
    public String getName() {return name;}
    @ConstructorParameters({"number", "name"})
    public NamedNumber(int number, String name) {
        this.number = number;
        this.name = name;
    }
    private final int number;
    private final String name;
}

And the equivalent with a record class:

public record NamedNumber(int number, String name) {}

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201
$ git checkout pull/3201

Update a local copy of the PR:
$ git checkout pull/3201
$ git pull https://git.openjdk.java.net/jdk pull/3201/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3201

View PR using the GUI difftool:
$ git pr show -t 3201

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3201.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 25, 2021

👋 Welcome back dfuchs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Mar 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@dfuch The following labels will be automatically applied to this pull request:

  • jmx
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 25, 2021

Loading

@dfuch
Copy link
Member Author

@dfuch dfuch commented Mar 25, 2021

/csr needed

Loading

@openjdk openjdk bot added the csr label Mar 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@dfuch has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@dfuch please create a CSR request and add link to it in JDK-8264124. This pull request cannot be integrated until the CSR request is approved.

Loading

<p>If the class is a {@link Record} and the getter is a component
accessor for a record component {@code name} of type <em>T</em>,
then the item in the {@code CompositeType} has the same name
than the record component, and has type <em>opentype(T)</em>.</p>
Copy link
Contributor

@AlanBateman AlanBateman Mar 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update to the MXBean spec looks good. Do you have a typo here, I assume it should say "as the record component".

Loading

Copy link
Member Author

@dfuch dfuch Mar 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks. Qualifying it as "a typo" was too kind ;-)
I have updated the PR. I also moved the record example after the from example to respect the order of precedence.

Loading

@ChrisHegarty
Copy link
Member

@ChrisHegarty ChrisHegarty commented Mar 26, 2021

Generally, I think that this is a good change. It nice to see that the semantics of record classes can be leverage to build the CompositeType in this way. And equally how that maps back to the canonical constructor.

Loading

@@ -753,7 +753,9 @@ another proxy (like {@code ModuleMXBean.getProduct()} which
{@code CompositeType} is determined by the <a href="#type-names">
type name rules</a> below.</p>
<p>The class is examined for getters using the conventions
<p>If the class is a {@link Record}, its getters are the
accessors for the record components. Otherwise, the
Copy link
Member

@mlchung mlchung Mar 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be good to add a link like {@linkplain RecordComponent record components}

Loading

Copy link
Member

@mlchung mlchung Mar 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You add record in "Mappings for other types". I think it deserves a separate section "Mappings for record classes" (maybe after primitive types). It's useful to add a row for record in the summary table above "Mappings for primitive types"

Loading

Copy link
Member Author

@dfuch dfuch Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link added. With respect to adding a section for records, it's a bit difficult to separate that from the "Mapping for other types" without a lot of duplication. I can add a row in the summary table, and then add a new section "Mapping for records" just before "Mapping for other types", that just gives a brief overview and refers to the mapping for other types below.

Something like this - what do you think?

    <h3 id="records">Mappings for Records</h3>

    <p>A {@linkplain Record record} <em>R</em> whose {@linkplain
      Class#getRecordComponents() components} are
      all convertible to open types, is itself convertible to a
      {@link CompositeType} as follows.
      The type name of this {@code CompositeType}
      is determined by the same <a href="#type-names">type name rules</a>
      defined by the <a href="#composite-map">Mapping for other types</a>
      below. Its getters are the accessors for the {@linkplain
      RecordComponent record components}, and the record is reconstructed
      using its canonical constructor, without needing any annotation.</p>

    <p>A record may also expose additional non-canonical constructors, which
      can be used to reconstruct the record if the composite data does
      not exactly contain all the components expected by the
      canonical constructor. However, in order to be taken into account
      by the MXBean framework, such non-canonical constructors
      need to be annotated with either the {@link ConstructorParameters
      &#64;javax.management.ConstructorParameters} or
     {@code @java.beans.ConstructorProperties} annotation.</p>

    <p>The complete rules for the mapping are detailed as part
      of the <a href="#composite-map">Mapping for other types</a>
      below.</p>

    <h3 id="composite-map">Mappings for other types</h3>

    <p>Given a record, or a Java class or interface <em>J</em> that does not match the other
      rules in the table above, the MXBean framework will attempt to map
      it to a {@link CompositeType} as follows.  [....]

Loading

Copy link
Member Author

@dfuch dfuch Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Mandy, I have updated the PR with a revised version of the text above. Is that more in line what you had in mind?

best regards,
-- daniel

Loading

Copy link
Member

@mlchung mlchung Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better. I like a separate mapping section for records since the mapping rules are straight-forward. The complexity comes if we want to support a record to implement CompositeDataView that allows a type to support more flexible conversion to CompositeData which is why you add records in the "Mappings for other types". I am wondering if we really want to support records to implement CompositeDataView but we may want to avoid such a special case.

I suggest add subsections in "Mappings for other types": There are two mapping rules: (1) opentype(J) maps J to CompositeType (2) opendata(J) maps J to CompositeData. opentype(J) for record class does not need to refer to other types. The common cases for opendata(J) for record class is using the canonical constructors. The other less common cases like using non-canonical constructors and CompositeDataView can state that it follows the same rules as specified for other types. "Mappings for other types" does not need any change for records.

"Reconstructing an instance of Java type J from a CompositeData" section should be renamed to "Reconstructing an instance of Java type or record class J from a CompositeData"

Here is a minor tweaking on "Mappings for Records"

A record is convertible to a CompositeType if and only if all its components are 
convertible to open types. Otherwise, it is not convertible.

A record class is converted to a CompositeType with one item for every record component as follows.
- The type name of this CompositeType is determined by the type name rules detailed below. 
- Each record component of type T, the item in the CompositeType has the same name 
  as the record component and of type opentype(T).

The mapping from an instance of a record class to a CompositeData corresponding to 
the CompositeType is the same as specified as for other types (add a link).

A record is reconstructed from a CompositeData using its canonical constructor. 
The canonical constructor doesn't require the presence of @javax.management.ConstructorParameters 
or @java.beans.ConstructorProperties annotations. If these annotations are present on
the canonical constructor, they will be ignored.

If the CompositeData from which the record is reconstructed doesn't contain all the record components, 
the MXBean framework will attempt to reconstruct the record in the same way as specified for 
other types (add a link).

Loading

Copy link
Member Author

@dfuch dfuch Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the PR with those changes, plus a few minor adjustments.

Loading

Copy link
Member

@mlchung mlchung left a comment

Thanks for making the change. The spec change looks good to me.

Loading

@openjdk openjdk bot removed the csr label Apr 7, 2021
@dfuch
Copy link
Member Author

@dfuch dfuch commented Apr 8, 2021

@mlchung @AlanBateman @ChrisHegarty would you formally approve the fix? The CSR has been approved.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

@dfuch This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records

Reviewed-by: mchung, chegar, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 95 new commits pushed to the master branch:

  • 627ad9f: 8262328: Templatize JVMFlag boilerplate access methods
  • c15680e: 8264868: Reduce inclusion of registerMap.hpp and register.hpp
  • 5784f6b: 8264948: Check for TLS extensions total length
  • 42f4d70: 8264649: runtime/InternalApi/ThreadCpuTimesDeadlock.java crash in fastdebug C2 with -XX:-UseTLAB
  • 76bd313: 8264872: Dependencies: Migrate to PerfData counters
  • 07c8ff4: 8264871: Dependencies: Miscellaneous cleanups in dependencies.cpp
  • 863feab: 8005295: Use mandated information for printing of repeating annotations
  • f26cd2a: 8264997: Remove SystemDictionary::cache_get
  • 9ebc497: 8264765: BreakIterator sees bogus sentence boundary in parenthesized “i.e.” phrase
  • ec31b3a: 8264727: Shenandoah: Remove extraneous whitespace from phase timings report
  • ... and 85 more: https://git.openjdk.java.net/jdk/compare/6c145c47683dd0073b9669acc24ed114443a50fa...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Apr 8, 2021
mlchung
mlchung approved these changes Apr 8, 2021
Copy link
Member

@mlchung mlchung left a comment

This looks okay to me but I'm not that familiar with the existing implementation. The test case covers the important cases.

Loading

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Spec and implementation changes look good.

Loading

@dfuch
Copy link
Member Author

@dfuch dfuch commented Apr 12, 2021

/integrate

Loading

@openjdk openjdk bot closed this Apr 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 12, 2021

@dfuch Since your change was applied there have been 109 commits pushed to the master branch:

  • 714ae54: 8258788: incorrect response to change in window insets [lanai]
  • f479437: 8265082: test/hotspot/jtreg/gc/g1/TestG1SkipCompaction.java fails validate-source
  • 27f4b27: 8264623: Change to Xcode 12.4 for building on Macos at Oracle
  • 7c20d97: 8265052: Break circular include dependency in objArrayOop.inline.hpp
  • b90ad76: 8233567: [TESTBUG] FocusSubRequestTest.java fails on macos
  • 125184e: 8265012: Shenandoah: Backout JDK-8264718
  • be0d46c: 8262068: Improve G1 Full GC by skipping compaction for regions with high survival ratio
  • f71be8b: 8264954: unified handling for VectorMask object re-materialization during de-optimization
  • 3c9858d: 8264827: Large mapped buffer/segment crash the VM when calling isLoaded
  • e604320: 8264783: G1 BOT verification should not verify beyond allocation threshold
  • ... and 99 more: https://git.openjdk.java.net/jdk/compare/6c145c47683dd0073b9669acc24ed114443a50fa...master

Your commit was automatically rebased without conflicts.

Pushed as commit d84a7e5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants