On Wed, 11 Jan 2023 11:29:39 GMT, Alexey Bakhtin <abakh...@openjdk.org> wrote:
>> 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.
>
> Alexey Bakhtin has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use copy of the ObjectName for matching
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 line 2024:
> 2022: // The other ObjectName is the string.
> 2023: return Util.wildmatch(name._canonicalName, _canonicalName,
> 2024: 0, 0, name.getDomainLength(), getDomainLength());
This looks wrong. Look at the parameters in wildmatch:
wildmatch(final String str, final String pat,
int stri, final int strend, int pati, final int patend)
What tests did you run to validate your changes? If none of the ObjectName
tests fail with your changes you will need to add a new test that catches the
error.
-------------
PR: https://git.openjdk.org/jdk/pull/11758
