Skip to content

Conversation

@marcosgopen
Copy link
Member

@marcosgopen marcosgopen commented Oct 14, 2025

CORE AS_TESTS RTS !JACOCO XTS QA_JTA QA_JTS_OPENJDKORB PERFORMANCE DB_TESTS

Improving Uid and utility performance

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

Benchmark output (please refer to the article https://developer.jboss.org/wiki/PerformanceGatesForAcceptingPerformanceFixesInNarayana for information on our testing procedures.

If you just want to run a single benchmark then please refer to the README.md file in our benchmark repository at https://github.com/jbosstm/performance/tree/main/narayana

For information on the hardware config used for this PR please consult the CI job artefact hwinfo.txt or the job output

1 similar comment
@jbosstm-bot
Copy link

Benchmark output (please refer to the article https://developer.jboss.org/wiki/PerformanceGatesForAcceptingPerformanceFixesInNarayana for information on our testing procedures.

If you just want to run a single benchmark then please refer to the README.md file in our benchmark repository at https://github.com/jbosstm/performance/tree/main/narayana

For information on the hardware config used for this PR please consult the CI job artefact hwinfo.txt or the job output

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@franz1981
Copy link
Contributor

@marcosgopen Anything more I should do here to help? 🙏
Thanks again for opening it, I was a bit scared of the codestyle :D

@marcosgopen marcosgopen marked this pull request as ready for review October 17, 2025 15:57
pos += secChars;
asciiBytes[pos++] = (byte) Uid.breakChar;
Utility.toHexChars(other, asciiBytes, pos, otherChars);
_stringForm = new String(asciiBytes, 0, 0, totalChars);
Copy link
Member Author

Choose a reason for hiding this comment

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

@franz1981 I can see this constructor is deprecated, could you use another constructor please?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's deprecated from more than 10 years (much more actually) and not planned (yet) to be removed
see https://docs.oracle.com/javase/1.5.0/docs/api/java/lang/String.html#String(byte[],%20int,%20int,%20int)
which is Java 5
Hotspot OpenJDK team is still working to make others as perfomant
see https://bugs.openjdk.org/browse/JDK-8295496 where I made the Performance OpenJDK team to work on it (actually I should work on it, but I have little time to fix it) - where there are benchmarks for the other versions.
Which make me think that _stringForm = new String(asciiBytes, 0, 0, totalChars); should be repalced by
_stringForm = new String(asciiBytes, 0);

@franz1981
Copy link
Contributor

FYI franz1981@739493b
based on what observed on benchmarks ^^

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@mmusgrov
Copy link
Member

Tests failed (https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-narayana-jdbcobjectstore/DB_TYPE=postgres,jdk=openJDK17,label=jnlp-agent/65/): Narayana rebase on main failed. Please rebase it manually

@franz1981 will you rebase your PR please.

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@mmusgrov
Copy link
Member

@franz1981 Thanks for rebasing. We have been having Jenkins CI problems recently, please bear with us while we investigate.

@franz1981
Copy link
Contributor

I will play in the weekend with some branchless algorithm but will hurt badly the readability, for a much higher throughput. Although tbh I don't think this one to be that slow compared to the openJDK one but will happily measure it

@franz1981
Copy link
Contributor

franz1981 commented Nov 1, 2025

Here we go, I have written it on a paper but not tried yet

private static long nibblesToAscii(long v) {
    long carry = ((v + 0x0606060606060606L) >>> 4) & 0x0101010101010101L;
    return v + 0x3030303030303030L + (carry * 0x27L);
}

This is fixing the encoded hex for nibbles mapping to a-f with the carry (which is 0 if 0-9 and 1 if a-f) multiplying per 0x27 the carry to produce a batch adjustment.
This approach has the downside of a multiplication but produce a single batch encoded hex which can be written in one go via VarHandle.
It cannot still be used as it is because we start with 8 bytes read and it could produce 16 ASCII hex chars, so we need a way to split the 8 bytes in low/hi nibbles, to use any of these methods here.
Clearly it is less maintainable but can be highly effective.
I will play with some benchmark for fun as I have some time.

And this one is saving the multiplication, too:

private static long nibblesToAscii(long v) {
    // For both cases: result = v + 0x30 + (v >= 10 ? 0x27 : 0)
    // But we can compute (v >= 10) as (v + 6) >>> 4 & 1
    long base = v + 0x3030303030303030L;
    long adjustment = ((v + 0x0606060606060606L) >>> 4) & 0x2727272727272727L;
    
    return base + adjustment;
}

@franz1981
Copy link
Contributor

Thinking about it twice since most of time we rarely encode the full 64 bit length is not worthy to have a swar variant

@mmusgrov
Copy link
Member

mmusgrov commented Nov 3, 2025

not worthy to have a swar variant

What's a swar variant?

@franz1981
Copy link
Contributor

The one based on #2414 (comment)

Which can process batches of bytes at a time

@marcosgopen
Copy link
Member Author

Thanks @franz1981 for updating your branch! I initially opened this PR to test your improvements but I think it makes more sense if we close this PR and we move the conversation to a new PR (same branch) opened by the branch's owner. Thanks again

@franz1981
Copy link
Contributor

@marcosgopen this is already from my branch

@marcosgopen
Copy link
Member Author

@marcosgopen this is already from my branch

Sorry, I can clarify: if you can open a new PR with this changes I can review it too. Otherwise I might seem the author of this PR's changes. Thanks

@franz1981
Copy link
Contributor

No worries @marcosgopen

Just a qq, I cannot see unit tests for Uid; I would like to write the one because the main difference with the new vs old way to encode hex is that I'm considering all long/int as unsigned - but Uid::minUid is actually negative, which means that I have to modify the util methods I created to work well with that

@marcosgopen
Copy link
Member Author

Closing in favor of #2427

@marcosgopen marcosgopen closed this Nov 4, 2025
@marcosgopen
Copy link
Member Author

marcosgopen commented Nov 4, 2025

No worries @marcosgopen

Just a qq, I cannot see unit tests for Uid; I would like to write the one because the main difference with the new vs old way to encode hex is that I'm considering all long/int as unsigned - but Uid::minUid is actually negative, which means that I have to modify the util methods I created to work well with that

@franz1981 you can find it here: https://github.com/jbosstm/narayana/blob/main/ArjunaCore/arjuna/tests/classes/com/hp/mwtests/ts/arjuna/uid/UidUnitTest.java

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