Skip zero init when copied array is the result of StringBuilder.toString #19370

ehsankianifar · 2024-04-23T18:33:20Z

StringBuilder.toString calls String(StringBuilder) which calls String(AbstractStringBuilder, void)
which calls Array.copyOfRange which calls
Array.copyOfRangeByte.
In this chain we can skip Zero alloc.

ehsankianifar · 2024-04-23T18:47:57Z

When StringBuilder.toString is called, it creates a new array and copy the string into that. Since the size of string builder value is always grater than or equal to the size of resulting string, we can allocate the copy array from the non zero TLH.
The call chain looks like this:

StringBuilder.toString() ->
   String<init>(StringBuilder) ->
      String<init>(AbstractStringBuilder, Void) ->
         Arrays.copyOfRange([BII) ->
            Arrays.copyOfRangeByte([BII) ->

It should be safe to just check if array is created in Arrays.copyOfRangeByte and the second caller is String<init>(AbstractStringBuilder, Void) since Arrays.copyOfRangeByte is a private method and is only called by Arrays.copyOfRange and also String<init>(AbstractStringBuilder, Void) is also a private constructor and called to convert StringBuilder to String.

This call chain is responsible for a significant portion of primitive array allocations and it increase non zero TLH utilization in some applications.

@r30shah

runtime/compiler/ilgen/Walker.cpp

ehsankianifar · 2024-04-23T19:46:15Z

runtime/compiler/ilgen/Walker.cpp

+      {
+      int32_t callerIndex = comp()->getCurrentInlinedCallSite()->_byteCodeInfo.getCallerIndex();
+      int32_t callerOfCallerIndex = comp()->getInlinedCallSite(callerIndex)._byteCodeInfo.getCallerIndex();
+      TR::ResolvedMethodSymbol *callerOfCaller = callerOfCallerIndex > -1 ? comp()->getInlinedResolvedMethodSymbol(callerOfCallerIndex) : comp()->getOptimizer()->getMethodSymbol();


When the index is -1, I thought I could use comp()->getMethodSymbol() but it returns the symbol for the inner most method (copyOfRangeByte) unless TR_DisableReturnCalleeInIlgen env var is set. It is funny that when I use comp()->getMethodSymbol() in dynamic debug counters it returns the outer most method symbol (index -1) but when use it here it returns the inner most method symbol! comp()->getOptimizer()->getMethodSymbol() seemingly works fine but I need to check with @r30shah

This question is still valid

ehsankianifar · 2024-04-23T19:49:09Z

runtime/compiler/ilgen/Walker.cpp

+      int32_t callerOfCallerIndex = comp()->getInlinedCallSite(callerIndex)._byteCodeInfo.getCallerIndex();
+      TR::ResolvedMethodSymbol *callerOfCaller = callerOfCallerIndex > -1 ? comp()->getInlinedResolvedMethodSymbol(callerOfCallerIndex) : comp()->getOptimizer()->getMethodSymbol();
+
+      if(callerOfCaller->getRecognizedMethod() == TR::java_lang_String_init_AbstractStringBuilder_Void)


I only check if the call chain is String(ASB, Void) -> Anything -> copyOfRangeByte the assumption is that the mission is to convert String builder to string when that pattern is observed but it may not be a safe assumption!

Can you elaborate bit more, what you meant by this assumption ?

I mean the purpose of String(ASB, Void) is to convert StringBuilder to String and the size of the resulting string can not be more than the string builder value. Therefore, when I see copyOfRangeByte with second caller String(ASB, Void) , I assume it is safe to skip zero init.

ehsankianifar · 2024-04-23T22:03:11Z

Started sanity tests in jenkins personal 21810 for jdk11 and jdk21 all systems with batch clear and dualTLH enabled

r30shah

Some initial thoughts. I will also checkout the call chain to evaluate if this is a safe to do or not.

runtime/compiler/ilgen/Walker.cpp

r30shah · 2024-04-24T01:54:17Z

runtime/compiler/ilgen/Walker.cpp

+      int32_t callerOfCallerIndex = comp()->getInlinedCallSite(callerIndex)._byteCodeInfo.getCallerIndex();
+      TR::ResolvedMethodSymbol *callerOfCaller = callerOfCallerIndex > -1 ? comp()->getInlinedResolvedMethodSymbol(callerOfCallerIndex) : comp()->getOptimizer()->getMethodSymbol();
+
+      if(callerOfCaller->getRecognizedMethod() == TR::java_lang_String_init_AbstractStringBuilder_Void)


Can you elaborate bit more, what you meant by this assumption ?

ehsankianifar · 2024-04-24T19:30:30Z

Tested for both UTF16 and Latin1 strings.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

runtime/compiler/control/DLLMain.cpp

ehsankianifar · 2024-05-06T23:27:22Z

Tested on all platform for java11 and java21 when batch clearing and dual TLH were enabled by default (21920). Most tests passed. The failed tests were unrelated to the changes and passed on retry.

ehsankianifar · 2024-05-06T23:27:52Z

@r30shah The pr is rebased and ready!

r30shah

Minor changes, Overall looks OK to me

runtime/compiler/ilgen/Walker.cpp

r30shah

Also, can we update the commit message with more useful information. I would emphasize the point that when a new array is allocated inside copyOfRangeByte which is inlined (In the call chain you are checking), it will write into complete array hence can skip allocation from zero memory

ehsankianifar · 2024-05-17T03:36:52Z

Thanks @r30shah for your review. I addressed all of your comments. Please let me know if the changes look good to you.

r30shah · 2024-06-10T20:23:21Z

jenkins test sanity all jdk21

r30shah

Change looks OK to me, some minor change in comment/function name. Please wait for build to finish before you make change and push it.

runtime/compiler/ilgen/Walker.cpp

When a new array is created in Arrays.copyOfRangeByte and it is inlined into: Arrays.copyOfRange -> String<init>(AbstractStringBuilder, Void) -> String<init>(StringBuilder) -> StringBuilder.toString() we can skip zero initialization because it writes the whole range of the array. Signed-off-by: Ehsan Kiani Far <[email protected]>

r30shah · 2024-06-11T13:34:59Z

FYI @vijaysun-omr - This change of Ehsan recognizes a call chain that was seen consistently Spring Petclinic app when ran in Liberty. As the part of the call chain when reaches to copyOfRangeByte allocates new array and writes it completely - It can be allocated from non zero initialized heap.

Re: #19370 (comment) - Build link :
https://openj9-jenkins.osuosl.org/view/Pull%20Requests/job/PullRequest-OpenJ9/5617/

r30shah · 2024-06-28T19:38:30Z

jenkins test sanity all jdk21

r30shah · 2024-07-04T14:54:23Z

Merging this change as all the tests passes.

ehsankianifar requested a review from dsouzai as a code owner April 23, 2024 18:33

ehsankianifar marked this pull request as draft April 23, 2024 18:33

ehsankianifar commented Apr 23, 2024

View reviewed changes

runtime/compiler/ilgen/Walker.cpp Outdated Show resolved Hide resolved

ehsankianifar commented Apr 23, 2024

View reviewed changes

runtime/compiler/ilgen/Walker.cpp Outdated Show resolved Hide resolved

ehsankianifar commented Apr 23, 2024

View reviewed changes

r30shah reviewed Apr 24, 2024

View reviewed changes

ehsankianifar force-pushed the SkipZeroTLHWhenStringBuilderToStringCopyArray branch 2 times, most recently from 49ba441 to b520865 Compare May 2, 2024 13:44

ehsankianifar commented May 2, 2024

View reviewed changes

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved

ehsankianifar commented May 2, 2024

View reviewed changes

runtime/compiler/control/DLLMain.cpp Outdated Show resolved Hide resolved

ehsankianifar force-pushed the SkipZeroTLHWhenStringBuilderToStringCopyArray branch from b520865 to de183e2 Compare May 6, 2024 23:21

ehsankianifar marked this pull request as ready for review May 6, 2024 23:24

knn-k added the comp:jit label May 7, 2024

r30shah self-assigned this May 10, 2024

r30shah requested changes May 10, 2024

View reviewed changes

runtime/compiler/ilgen/Walker.cpp Outdated Show resolved Hide resolved

runtime/compiler/ilgen/Walker.cpp Outdated Show resolved Hide resolved

runtime/compiler/ilgen/Walker.cpp Outdated Show resolved Hide resolved

r30shah reviewed May 10, 2024

View reviewed changes

ehsankianifar force-pushed the SkipZeroTLHWhenStringBuilderToStringCopyArray branch from de183e2 to d4fcd33 Compare May 16, 2024 15:38

ehsankianifar force-pushed the SkipZeroTLHWhenStringBuilderToStringCopyArray branch from d4fcd33 to aa99067 Compare May 22, 2024 15:06

r30shah reviewed Jun 10, 2024

View reviewed changes

runtime/compiler/ilgen/Walker.cpp Outdated Show resolved Hide resolved

runtime/compiler/ilgen/Walker.cpp Outdated Show resolved Hide resolved

ehsankianifar force-pushed the SkipZeroTLHWhenStringBuilderToStringCopyArray branch from aa99067 to 03850dd Compare June 11, 2024 13:03

r30shah merged commit 289a133 into eclipse-openj9:master Jul 4, 2024
17 checks passed

ehsankianifar deleted the SkipZeroTLHWhenStringBuilderToStringCopyArray branch July 15, 2024 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip zero init when copied array is the result of StringBuilder.toString #19370

Skip zero init when copied array is the result of StringBuilder.toString #19370

ehsankianifar commented Apr 23, 2024

ehsankianifar commented Apr 23, 2024 •

edited

Loading

ehsankianifar Apr 23, 2024

ehsankianifar May 9, 2024

ehsankianifar Apr 23, 2024

r30shah Apr 24, 2024

ehsankianifar Apr 24, 2024

ehsankianifar commented Apr 23, 2024

r30shah left a comment

r30shah Apr 24, 2024

ehsankianifar commented Apr 24, 2024

ehsankianifar commented May 6, 2024

ehsankianifar commented May 6, 2024

r30shah left a comment

r30shah left a comment

ehsankianifar commented May 17, 2024

r30shah commented Jun 10, 2024

r30shah left a comment

r30shah commented Jun 11, 2024

r30shah commented Jun 28, 2024

r30shah commented Jul 4, 2024

Skip zero init when copied array is the result of StringBuilder.toString #19370

Skip zero init when copied array is the result of StringBuilder.toString #19370

Conversation

ehsankianifar commented Apr 23, 2024

ehsankianifar commented Apr 23, 2024 • edited Loading

ehsankianifar Apr 23, 2024

Choose a reason for hiding this comment

ehsankianifar May 9, 2024

Choose a reason for hiding this comment

ehsankianifar Apr 23, 2024

Choose a reason for hiding this comment

r30shah Apr 24, 2024

Choose a reason for hiding this comment

ehsankianifar Apr 24, 2024

Choose a reason for hiding this comment

ehsankianifar commented Apr 23, 2024

r30shah left a comment

Choose a reason for hiding this comment

r30shah Apr 24, 2024

Choose a reason for hiding this comment

ehsankianifar commented Apr 24, 2024

ehsankianifar commented May 6, 2024

ehsankianifar commented May 6, 2024

r30shah left a comment

Choose a reason for hiding this comment

r30shah left a comment

Choose a reason for hiding this comment

ehsankianifar commented May 17, 2024

r30shah commented Jun 10, 2024

r30shah left a comment

Choose a reason for hiding this comment

r30shah commented Jun 11, 2024

r30shah commented Jun 28, 2024

r30shah commented Jul 4, 2024

ehsankianifar commented Apr 23, 2024 •

edited

Loading