Skip to content

Conversation

@franz1981
Copy link
Contributor

@franz1981 franz1981 commented Nov 4, 2025

https://issues.redhat.com/browse/JBTM-4014

CORE !AS_TESTS !RTS !JACOCO !XTS !QA_JTA !QA_JTS_OPENJDKORB PERFORMANCE !DB_TESTS

This pull request refactors the Uid class and related utility methods to improve performance and memory usage by replacing stream-based serialization/deserialization with direct byte array manipulation and optimized hex string conversion. The changes also introduce new helper methods in Utility.java for more efficient hex conversions, and update usages in Uid.java to leverage these helpers.

The most important changes are:

Performance and Memory Optimization:

  • Replaced ByteArrayInputStream/DataInputStream and ByteArrayOutputStream/DataOutputStream with direct byte array access using VarHandle for reading and writing UID fields in Uid.java. This avoids object allocation and improves performance. [1] [2]
  • Introduced AtomicReferenceFieldUpdater for _byteForm to ensure thread-safe lazy initialization of UID byte representation.

Hex String Conversion Improvements:

  • Added static helper methods (hexCharsOf, toHexChars) to Utility.java for efficiently determining hex string length and converting int/long values to hex ASCII bytes, replacing previous string-based conversions.
  • Updated Uid.stringForm() and Uid.fileStringForm() to use the new byte-based hex conversion helpers, improving speed and reducing allocations.

Code Modernization and Consistency:

  • Changed break character constants in Uid.java from char to byte for consistency with new byte array logic.
  • Added a static array of hex digit bytes in Utility.java for fast lookup during hex conversions.

@jbosstm-bot
Copy link

⚠️ narayana CI not started.

Author is not the 'narayana' contributor, to permit PR being run members of jbosstm can write comment of text: TESTIT

@franz1981
Copy link
Contributor Author

franz1981 commented Nov 4, 2025

@marcosgopen I've removed the existing long based utility method because not used anymore - but we still use the int variant.

One note:

Uid can creates instances with always the same

            hostAddr = Utility.hostInetAddr(); /* calculated only once */
            process = Utility.getpid();

why not lazily encode and cache both hex values for these?
If is a very frequent case, it would save hex encoding both.

@marcosgopen
Copy link
Member

TESTIT

@marcosgopen
Copy link
Member

@mmusgrov
Copy link
Member

mmusgrov commented Nov 4, 2025

@marcosgopen I've removed the existing long based utility method because not used anymore - but we still use the int variant.

One note:

Uid can creates instances with always the same

            hostAddr = Utility.hostInetAddr(); /* calculated only once */
            process = Utility.getpid();

why not lazily encode and cache both hex values for these? If is a very frequent case, it would save hex encoding both.

Yeah, I notice that while reviewing the PR. We should see if it makes a difference to the benchmarks and then follow up with the change.

mmusgrov
mmusgrov previously approved these changes Nov 4, 2025
Copy link
Member

@mmusgrov mmusgrov left a comment

Choose a reason for hiding this comment

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

I think it's a good PR and the performance improvement is very worthwhile. I learnt some new things and although I did not fully understand the hex char manipulations in Utility.java they are clearly working.

I forgot to wait for the CI run, I will trigger one manually.

@jbosstm-bot
Copy link

CORE profile tests failed (https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-narayana/PROFILE=CORE,jdk=openJDK17,label=jnlp-agent/46/): Narayana rebase on main failed. Please rebase it manually

@mmusgrov
Copy link
Member

mmusgrov commented Nov 5, 2025

CORE profile tests failed (https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-narayana/PROFILE=CORE,jdk=openJDK17,label=jnlp-agent/46/): Narayana rebase on main failed. Please rebase it manually

Sorry @franz1981 but CI is reporting that the PR needs a rebase.

@mmusgrov mmusgrov dismissed their stale review November 5, 2025 10:10

Waiting for CI to complete


// check the min/max string and file string forms

assertEquals("-8000000000000000:-8000000000000000:-80000000:-80000000:-80000000", minUid.stringForm());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added these since relevant for the hex encoding @marcosgopen

@jbosstm-bot
Copy link

jbosstm-bot commented Nov 5, 2025

⚠️ narayana CI not started.

Author is not the 'narayana' contributor, to permit PR being run members of jbosstm can write comment of text: TESTIT

[edit by mmusgrov] I triggered build 49 and the 51

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

Copy link
Member

@mmusgrov mmusgrov left a comment

Choose a reason for hiding this comment

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

Resubmitting the review since the CI run passed..

Copy link
Member

@mmusgrov mmusgrov left a comment

Choose a reason for hiding this comment

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

Resubmitting the review since the CI run passed..

Copy link
Member

@marcosgopen marcosgopen left a comment

Choose a reason for hiding this comment

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

I am approving too after having tested the improvement locally. See my comment jbosstm/performance#185 (comment)

@mmusgrov mmusgrov merged commit 8106679 into jbosstm:main Nov 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants