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

Use non zero tlh for int and long to string #19471

Conversation

ehsankianifar
Copy link
Contributor

Converting numbers to string does nor require zero memory allocation as it overwrite the whole memory. The Integer.toString is an intrinsic candidate which makes it safer to do this optimization. We inline the "getChars" method for both Integer and Long classes here which does the to string conversion!
I looked into some real world applications and this change does not have a significant impact on nonZeroTLH utilization but since it is easy enough, I decided to raise this PR.
@r30shah Please let me know if this change looks okay to you.

@@ -356,10 +356,6 @@ IDATA J9VMDllMain(J9JavaVM* vm, IDATA stage, void * reserved)
/* The order is important: we have to request it before the first TLH is created. */
/* Platform detection seems difficult at this point. */
{
#ifdef TR_HOST_X86

This comment was marked as resolved.

@ehsankianifar ehsankianifar marked this pull request as draft May 9, 2024 15:23
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Thanks @ehsankianifar Looks Good to me. I think we can get these changes merged safely. Can you remove draft from the PR and also remove the other commit you have added for me to launch tests?

@ehsankianifar ehsankianifar force-pushed the UseNonZeroTlhForIntAndLongToString branch from b11cad0 to de09ce6 Compare May 10, 2024 23:44
@ehsankianifar ehsankianifar marked this pull request as ready for review May 10, 2024 23:45
@ehsankianifar
Copy link
Contributor Author

Thanks @r30shah . I ran sanity tests on all platforms with JDK11 and JDK21 (22063). Still running but no relevant failure has been observed so far.
Removed the test commit and draft tag as you requested.

@r30shah
Copy link
Contributor

r30shah commented May 11, 2024

jenkins test sanity all jdk21,jdk11

@r30shah
Copy link
Contributor

r30shah commented May 11, 2024

Hi @ehsankianifar If you have launched internal tests with JDK11 and Jdk21 PPCLE linux, can you share results? If not, can you launch one?

@ehsankianifar
Copy link
Contributor Author

Thanks @r30shah. I ran personal tests for jdk11 and 21 on all platforms with dual TLH and batch clearing enabled (Personal/22063). All tests passed on Linux_x86-64, Linux_ppc64le, Linux_s390x, Linux_aarch64 and Mac_aarch64.

A few timeout and failures observed on windows, aix, and x64_mac but not related to these changes.

@r30shah
Copy link
Contributor

r30shah commented May 13, 2024

Thanks @ehsankianifar ,
Merging PR based on the testing done in #19471 (comment) and on the builds.

One more nit pick - Can you update the commit title and I will merge the changes.

Add Int/Long.toString method to list skipping zero init on new arrays

Converting numbers to string does not require zeroed memory.
In case of int and long we already inline the methods.
Also the int.toString is an intrinsic candidate.

Signed-off-by: Ehsan Kiani Far <ehsan.kianifar@gmail.com>
@ehsankianifar ehsankianifar force-pushed the UseNonZeroTlhForIntAndLongToString branch from de09ce6 to c9be248 Compare May 13, 2024 20:33
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Merging based on the testing done so far.

@r30shah r30shah merged commit eedd850 into eclipse-openj9:master May 13, 2024
2 checks passed
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.

None yet

2 participants