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
Use non zero tlh for int and long to string #19471
Conversation
runtime/compiler/control/DLLMain.cpp
Outdated
@@ -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.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this 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?
b11cad0
to
de09ce6
Compare
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. |
jenkins test sanity all jdk21,jdk11 |
Hi @ehsankianifar If you have launched internal tests with JDK11 and Jdk21 PPCLE linux, can you share results? If not, can you launch one? |
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. |
Thanks @ehsankianifar , One more nit pick - Can you update the commit title and I will merge the changes.
|
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>
de09ce6
to
c9be248
Compare
There was a problem hiding this 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.
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.