8299234: JMX Repository.query performance #11758

alexeybakhtin · 2022-12-21T18:50:52Z

Please find a patch to improve JMX Repository.query performance

Using ObjectName.apply() allows significantly decrease memory usage and the number of GC cycles:
Before:

$ java test 1000000 1000000
Test PASSED in 8943169791 ns.
GC: G1 Young Generation getCollectionCount()=177 getCollectionTime()=118

After:

$ java test 1000000 1000000
Test PASSED in 4808213917 ns.
GC: G1 Young Generation getCollectionCount()=88 getCollectionTime()=53

Private ObjectName.matchDomains() method is also updated to minimize unnecessary memory allocation.

All javax/management jtreg tests passed successfully.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8299234: JMX Repository.query performance

Reviewers

Daniel Fuchs (@dfuch - Reviewer)
Kevin Walls (@kevinjwalls - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11758/head:pull/11758
$ git checkout pull/11758

Update a local copy of the PR:
$ git checkout pull/11758
$ git pull https://git.openjdk.org/jdk pull/11758/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11758

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11758.diff

bridgekeeper · 2022-12-21T18:52:15Z

👋 Welcome back abakhtin! 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.

openjdk · 2022-12-21T18:55:30Z

@alexeybakhtin The following label will be automatically applied to this pull request:

serviceability

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

mlbridge · 2022-12-21T18:58:49Z

Webrevs

kevinjwalls · 2023-01-06T11:17:05Z

Hi,
Speedup sounds good.
So we don't need the ObjectNamePattern class, and it's private so no dependencies.
I haven't fully followed the wildcard matching changes yet.
Will try and do some testing also.

dfuch · 2023-01-10T17:14:29Z

I see several issues with the proposed changes: ObjectName is not final, so I'm not sure the changes proposed to ObjectName are safe when/if a subclass is supplied. And in addition, because it's not final, you will need to make a defensive copy of the input parameter in Repository::query and do the matching against the copy.

alexeybakhtin · 2023-01-11T11:24:55Z

I see several issues with the proposed changes: ObjectName is not final, so I'm not sure the changes proposed to ObjectName are safe when/if a subclass is supplied. And in addition, because it's not final, you will need to make a defensive copy of the input parameter in Repository::query and do the matching against the copy.

Hi @dfuch, Thank you for review!
ObjectName is not final but it creates _canonicalName for all subclasses and uses it for object equality. You can find examples of usage in the ObjectName::apply and ObjectName::equals. So, I think it should be OK to use it in ObjectName::matchDomains also.
I'm agree, that creating copy of the input parameters in Repository::query is safer. I have updated this part.

Even with defensive copy the performance is improved for ObjectName subclasses:
New code:
Test PASSED in 9468199583 ns.
GC: G1 Young Generation getCollectionCount()=149 getCollectionTime()=95
vs
Original code:
Test PASSED in 13975035958 ns.
GC: G1 Young Generation getCollectionCount()=304 getCollectionTime()=155

dfuch

Using ObjectName::getInstance for the defensive copy is the right choice since it will only make a copy if the argument is a subclass. That looks good. I see your point about using _canonical_name. Technically this is still an observable change of behaviour - so it might be preferable to file a CSR if you want to proceed with the changes made to ObjectName.
Regardless of that these changes seem to be introducing a bug (see my other comment inline).

src/java.management/share/classes/javax/management/ObjectName.java

dfuch · 2023-01-23T14:14:50Z

Hi Alexey - you have addressed most of my concerns but let me reinstate that the changes to ObjectName are observable by subclasses and thus will require a CSR. As such this fix may also not be a good candidate for backport (if that was your intention), as it comes with a potential regression risk. A possibility could be to implement the ObjectName changes as a separate PR.

alexeybakhtin · 2023-01-23T20:25:00Z

Hi Daniel, Yes, I'd like to backport these changes to JDK11 and 8. Such modifications to the OjectName could be unacceptable risky. I have reverted all changes in the ObjectName class.
Unfortunately, creating an extra string affects performance, but it is still better than before.
Before:

$ java test 1000000 1000000
Test PASSED in 8943169791 ns.
GC: G1 Young Generation getCollectionCount()=177 getCollectionTime()=118

After:

$ java test 1000000 1000000
Test PASSED in 6781538084 ns.
GC: G1 Young Generation getCollectionCount()=141 getCollectionTime()=77

dfuch

I don't see an obvious issue with the proposed changes. Do you have a JMH benchmark to prove the new code is faster? Also please obtain a review from a maintainer of the servicieability area before integrating.

alexeybakhtin · 2023-02-02T13:56:59Z

@dfuch, I've returned ObjectName optimization for the instances of ObjectName class only.
JMH benchmark does not show much difference without these changes.
Benchmark results for original implementation:

Benchmark                                 (N)   Mode  Cnt    Score    Error  Units
JmxBenchmark.applyForMyObjectName        1000  thrpt   10  539.134 ± 11.643  ops/s
JmxBenchmark.applyForObjectName          1000  thrpt   10  495.961 ± 10.550  ops/s
JmxBenchmark.queryMBeansForMyObjectName  1000  thrpt   10   75.482 ±  0.302  ops/s
JmxBenchmark.queryMBeansForObjectName    1000  thrpt   10  111.721 ±  0.462  ops/s
JmxBenchmark.queryNamesForMyObjectName   1000  thrpt   10   79.004 ±  0.398  ops/s
JmxBenchmark.queryNamesForObjectName     1000  thrpt   10  125.877 ±  2.024  ops/s

Benchmark results for the proposed implementation:

Benchmark                                 (N)   Mode  Cnt    Score    Error  Units
JmxBenchmark.applyForMyObjectName        1000  thrpt   10  542.233 ±  4.137  ops/s
JmxBenchmark.applyForObjectName          1000  thrpt   10  668.710 ± 13.124  ops/s
JmxBenchmark.queryMBeansForMyObjectName  1000  thrpt   10  123.381 ±  4.254  ops/s
JmxBenchmark.queryMBeansForObjectName    1000  thrpt   10  237.607 ±  3.298  ops/s
JmxBenchmark.queryNamesForMyObjectName   1000  thrpt   10  122.576 ±  2.364  ops/s
JmxBenchmark.queryNamesForObjectName     1000  thrpt   10  238.275 ±  4.904  ops/s

JmxBenchmark.applyForMyObjectName - tests ObjectName.apply() for subclasses of ObjcetName instances
JmxBenchmark.applyForObjectName - tests ObjectName.apply() for ObjectName instances
JmxBenchmark.queryMBeansForMyObjectName - tests MBeanServer.queryMBeans() for subclasses of ObjcetName instances
JmxBenchmark.queryMBeansForObjectName - tests MBeanServer.queryMBeans() for ObjcetName instances
JmxBenchmark.queryNamesForMyObjectName - tests MBeanServer.queryNames() for subclasses of ObjcetName instances
JmxBenchmark.queryNamesForObjectName - tests MBeanServer.queryNames() for ObjcetName instances

JmxBenchmark is attached to the https://bugs.openjdk.org/browse/JDK-8299234

@sspitsyn, Could you please look at the proposed enhancement

alexeybakhtin · 2023-02-13T14:47:03Z

@sspitsyn, Could you take a look at the proposed enhancement or suggest someone who can review it? Thank you

alexeybakhtin · 2023-02-15T13:46:21Z

@AlanBateman, Could you please take a look at the proposed performance improvement? It is required to reduce high CPU utilization caused by unnecessary HashTable cloning in the ObjectName::getKeyPropertyList(). I also would like to backport it to 17, 11 and 8 if the changes are acceptable.
Thank you in advance

sspitsyn · 2023-02-16T08:16:40Z

Hi Alexey,
I hope, Kevin will have a chance to review your PR. I'll check with him.

alexeybakhtin · 2023-02-16T08:19:12Z

Hi Alexey, I hope, Kevin will have a chance to review your PR. I'll check with him.

Thank you!

kevinjwalls · 2023-02-17T17:33:19Z

Yes, I think it looks good.
I looked a while ago but it was somewhat unfamiliar so was glad of Daniel's input.

It's a bit of a style nit but our ifs always use brackets, so I miss that in ObjectName.matchDomains(), particularly as the statements are multi-line.

I tested the change with test/jdk/javax/management/ and everything still passes.
The repo is somewhat behind, but these changes have no merge problems with the latest. I took these changes into a latest jdk repo to build and test.

src/java.management/share/classes/javax/management/ObjectName.java

alexeybakhtin · 2023-02-18T13:54:27Z

/integrate

openjdk · 2023-02-18T13:54:52Z

@alexeybakhtin This pull request has not yet been marked as ready for integration.

alexeybakhtin · 2023-02-20T08:10:49Z

@kevinjwalls, Thank you very much for reviewing.
@sspitsyn, the patch is approved by Kevin but unfortunately, he is not a reviewer in JDK project. Could you please approve it or ask somebody from the serviceability group? Thank you in advance.

sspitsyn · 2023-02-20T10:36:27Z

@kevinjwalls, Thank you very much for reviewing. @sspitsyn, the patch is approved by Kevin but unfortunately, he is not a reviewer in JDK project. Could you please approve it or ask somebody from the serviceability group? Thank you in advance.

Alexey, two reviews are normally required before integration in the Serviceability area.
I guess, you need to wait for review from Daniel Fuchs (an expert in this area) or someone else.

dfuch

This last version LGTM

openjdk · 2023-02-20T17:16:26Z

@alexeybakhtin 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:

8299234: JMX Repository.query performance

Reviewed-by: dfuchs, kevinw

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 824 new commits pushed to the master branch:

e47e9ec: 8300658: memory_and_swap_limit() reporting wrong values on systems with swapaccount=0
7cf7e0a: 8302070: Factor null-check into load_klass() calls
e731695: 8302882: Newly added test javax/swing/JFileChooser/JFileChooserFontReset.java fails with HeadlessException
b5a7426: 8301749: Tracking malloc pooled memory size
6ac5e05: 8302068: Serial: Refactor oop closures used in Young GC
71cf7c4: 8302518: Add missing Op_RoundDoubleMode in VectorNode::vector_operands()
98716e2: 8302709: Remove explicit remembered set verification in G1
303c61f: 8302776: RISC-V: Fix typo CSR_INSTERT to CSR_INSTRET
7c40c8a: 8302312: Make ParGCRareEvent_lock G1 specific
593bec6: 8302122: Parallelize TLAB retirement in prologue in G1
... and 814 more: https://git.openjdk.org/jdk/compare/a7d6de71bb83c8715654f61dd166aad6e8dab847...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.

alexeybakhtin · 2023-02-20T17:51:48Z

/integrate

openjdk · 2023-02-20T17:53:01Z

Going to push as commit 9a79722.
Since your change was applied there have been 824 commits pushed to the master branch:

e47e9ec: 8300658: memory_and_swap_limit() reporting wrong values on systems with swapaccount=0
7cf7e0a: 8302070: Factor null-check into load_klass() calls
e731695: 8302882: Newly added test javax/swing/JFileChooser/JFileChooserFontReset.java fails with HeadlessException
b5a7426: 8301749: Tracking malloc pooled memory size
6ac5e05: 8302068: Serial: Refactor oop closures used in Young GC
71cf7c4: 8302518: Add missing Op_RoundDoubleMode in VectorNode::vector_operands()
98716e2: 8302709: Remove explicit remembered set verification in G1
303c61f: 8302776: RISC-V: Fix typo CSR_INSTERT to CSR_INSTRET
7c40c8a: 8302312: Make ParGCRareEvent_lock G1 specific
593bec6: 8302122: Parallelize TLAB retirement in prologue in G1
... and 814 more: https://git.openjdk.org/jdk/compare/a7d6de71bb83c8715654f61dd166aad6e8dab847...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-20T17:53:18Z

@alexeybakhtin Pushed as commit 9a79722.

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

openjdk · 2023-02-21T09:31:01Z

@sspitsyn The command sponsor can only be used in open pull requests.

8299234: JMX Repository.query performance

c60e6b6

openjdk bot added the rfr Pull request is ready for review label Dec 21, 2022

openjdk bot added the serviceability serviceability-dev@openjdk.org label Dec 21, 2022

Use copy of the ObjectName for matching

1ecc5ad

dfuch reviewed Jan 11, 2023

View reviewed changes

src/java.management/share/classes/javax/management/ObjectName.java Outdated Show resolved Hide resolved

Fix parameters order for Util.wildmatch

5ac859a

Revert changes in the ObjectName

594dfa5

alexeybakhtin requested a review from dfuch January 30, 2023 12:44

dfuch reviewed Jan 30, 2023

View reviewed changes

Optimize ObjectName.matchDomains

0bdbbcb

kevinjwalls reviewed Feb 17, 2023

View reviewed changes

src/java.management/share/classes/javax/management/ObjectName.java Outdated Show resolved Hide resolved

Add braces

32259a1

kevinjwalls approved these changes Feb 17, 2023

View reviewed changes

alexeybakhtin requested a review from dfuch February 20, 2023 08:39

dfuch approved these changes Feb 20, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 20, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 20, 2023

openjdk bot closed this Feb 20, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8299234: JMX Repository.query performance #11758

8299234: JMX Repository.query performance #11758

alexeybakhtin commented Dec 21, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 21, 2022

openjdk bot commented Dec 21, 2022

mlbridge bot commented Dec 21, 2022 •

edited

Loading

kevinjwalls commented Jan 6, 2023

dfuch commented Jan 10, 2023

alexeybakhtin commented Jan 11, 2023 •

edited

Loading

dfuch left a comment •

edited

Loading

dfuch commented Jan 23, 2023 •

edited

Loading

alexeybakhtin commented Jan 23, 2023 •

edited

Loading

dfuch left a comment

alexeybakhtin commented Feb 2, 2023

alexeybakhtin commented Feb 13, 2023

alexeybakhtin commented Feb 15, 2023

sspitsyn commented Feb 16, 2023

alexeybakhtin commented Feb 16, 2023

kevinjwalls commented Feb 17, 2023

alexeybakhtin commented Feb 18, 2023

openjdk bot commented Feb 18, 2023

alexeybakhtin commented Feb 20, 2023

sspitsyn commented Feb 20, 2023

dfuch left a comment

openjdk bot commented Feb 20, 2023

alexeybakhtin commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

openjdk bot commented Feb 21, 2023

8299234: JMX Repository.query performance #11758

8299234: JMX Repository.query performance #11758

Conversation

alexeybakhtin commented Dec 21, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 21, 2022

openjdk bot commented Dec 21, 2022

mlbridge bot commented Dec 21, 2022 • edited Loading

Webrevs

kevinjwalls commented Jan 6, 2023

dfuch commented Jan 10, 2023

alexeybakhtin commented Jan 11, 2023 • edited Loading

dfuch left a comment • edited Loading

Choose a reason for hiding this comment

dfuch commented Jan 23, 2023 • edited Loading

alexeybakhtin commented Jan 23, 2023 • edited Loading

dfuch left a comment

Choose a reason for hiding this comment

alexeybakhtin commented Feb 2, 2023

alexeybakhtin commented Feb 13, 2023

alexeybakhtin commented Feb 15, 2023

sspitsyn commented Feb 16, 2023

alexeybakhtin commented Feb 16, 2023

kevinjwalls commented Feb 17, 2023

alexeybakhtin commented Feb 18, 2023

openjdk bot commented Feb 18, 2023

alexeybakhtin commented Feb 20, 2023

sspitsyn commented Feb 20, 2023

dfuch left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 20, 2023

alexeybakhtin commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

openjdk bot commented Feb 21, 2023

alexeybakhtin commented Dec 21, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 21, 2022 •

edited

Loading

alexeybakhtin commented Jan 11, 2023 •

edited

Loading

dfuch left a comment •

edited

Loading

dfuch commented Jan 23, 2023 •

edited

Loading

alexeybakhtin commented Jan 23, 2023 •

edited

Loading