[PING] RFR(M): 8207343: Automate vtable/itable stub size calculation

Schmidt, Lutz lutz.schmidt at sap.com
Thu Aug 16 07:53:44 UTC 2018


Sorry for spamming: forgot to attach the output file.
Lutz

On 16.08.18, 09:51, "Schmidt, Lutz" <lutz.schmidt at sap.com> wrote:

    Hi Vladimir, 
    
    I have reformatted the trace output a bit, see attached file. Do you like it better now? And the printed actual size was plain wrong, it is now calculated as (pc - code_begin).
    
    Any other comments?
    Thanks,
    Lutz
    
    On 15.08.18, 18:25, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
    
        It looks good to me only one suggestion is to add 'size' to be clear what these numbers mean:
        
        vtable #3 at 0x0000000116e85c30[1024], size estimate 1024, left over: 353
        
        Also looking on numbers and it strange. Size estimate 'stub_length' matches actual size (code_end - entry_point) then 
        why left over?
        
        Thanks,
        Vladimir
        
        On 8/15/18 5:55 AM, Schmidt, Lutz wrote:
        > Hi Vladimir,
        > what do you think about this change (see attachment) to convert tracing output to UL? I will create a new webrev once I have included your (expected) comments.
        > Regards, Lutz
        > 
        > On 15.08.18, 00:34, "Schmidt, Lutz" <lutz.schmidt at sap.com> wrote:
        > 
        >      No hurry, Vladimir!
        >      In the meantime, I will have a look at the PrintMiscellaneous to UL conversion. In the light of previous discussions, this might help making some people happy. __
        >      Thanks,
        >      Lutz
        >      
        >      On 15.08.18, 00:25, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
        >      
        >          On 8/14/18 12:46 AM, Schmidt, Lutz wrote:
        >          > Hi Vladimir,
        >          >
        >          > the answer is simple: No, I did not. The proposed change is basically a generalized, harmonized variant of a modification we have been using at SAP for many years.
        >          
        >          Okay. Give me time to look and test changes.
        >          
        >          What do you think about using Xlog (UL) instead of PrintMiscellaneous in bookkeeping()?
        >          
        >          >
        >          > Your idea is interesting, though. I (still) do not completely like the implementation currently in RFR. The code size variance caused by data only known at runtime (constants, offsets, for example) makes the generators ugly.
        >          >
        >          > Using a temp buffer adds other complexities. What about relocations, for example. Without having had a deeper look, I expect the effort to be "considerable".
        >          
        >          Yes, it is not simple change.
        >          
        >          Thanks,
        >          Vladimir
        >          
        >          >
        >          > Thanks,
        >          > Lutz
        >          >
        >          > On 14.08.18, 00:49, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
        >          >
        >          >      Hi Lutz,
        >          >
        >          >      Did you consider to generate these stubs in temp buffer before publishing them in CodeCache as we do
        >          >      for nmethods?
        >          >
        >          >      Thanks,
        >          >      Vladimir
        >          >
        >          >      On 8/13/18 3:52 AM, Schmidt, Lutz wrote:
        >          >      > Dear Community,
        >          >      > are there any praises, objections, questions, comments, or even reviews for this change?
        >          >      > Thanks for considering!
        >          >      > Lutz
        >          >      >
        >          >      > On 13.08.18, 12:47, "Schmidt, Lutz" <lutz.schmidt at sap.com> wrote:
        >          >      >
        >          >      >      Hi Boris,
        >          >      >
        >          >      >      back from vacation I'd like to elaborate a bit more on your comments.
        >          >      >
        >          >      >      Based on your input, I have changed the description in VtableStub::pd_code_alignment() to read
        >          >      >        "ARM32 cache line size is not an architected constant. We just align on word size."
        >          >      >
        >          >      >      The description for aarch64 was adapted accordingly:
        >          >      >        "aarch64 cache line size is not an architected constant. We just align on 4 bytes (instruction size)."
        >          >      >
        >          >      >      With respect to the variable name, I just harmonized the naming across all platforms. I would appreciate if you could live with it. Another option would be to just return a value, without using any variable name. I don't like that too much, but if the community prefers it...
        >          >      >
        >          >      >      I have updated http://cr.openjdk.java.net/~lucy/webrevs/8207343.01 in-place with just the two comment lines modified.
        >          >      >
        >          >      >      Thank you,
        >          >      >      Lutz
        >          >      >
        >          >      >      On 07.08.18, 22:12, "Schmidt, Lutz" <lutz.schmidt at sap.com> wrote:
        >          >      >
        >          >      >          Hi Boris,
        >          >      >          thanks for looking at this. I will respond to your comments in more detail next week. I'm on vacation this week.
        >          >      >          Thanks,
        >          >      >          Lutz
        >          >      >
        >          >      >          On 05.08.18, 19:08, "Boris Ulasevich" <boris.ulasevich at bell-sw.com> wrote:
        >          >      >
        >          >      >              Hi Lutz,
        >          >      >
        >          >      >                 I have run jtreg with your change and do not see new fails (test that
        >          >      >              fails on aarch64 is excluded for 32 bit platforms).
        >          >      >
        >          >      >                 I am OK with your change (I'm not a reviewer). But description and
        >          >      >              variable name in VtableStub::pd_code_alignment function looks strange
        >          >      >              for me. Raspberry Pi2 ARM1176JZF-S processor has a cache line length of
        >          >      >              32 bytes, and, as I know, icache line size is not a constant for ARM32
        >          >      >              architecture.
        >          >      >
        >          >      >              regards,
        >          >      >              Boris
        >          >      >
        >          >      >              On 02.08.2018 15:02, Schmidt, Lutz wrote:
        >          >      >              > Hi Zhongwei,
        >          >      >              >
        >          >      >              > thank you for testing aarch64. Given my lack of expertise, I am surprised there are only those two issues.
        >          >      >              > Ad 1.: fixed. The declaration just a few lines above was forgotten to adapt.
        >          >      >              > Ad 2.: adapted. I pushed the estimate to 152 bytes. I expected this value would require some adjustment.
        >          >      >              >
        >          >      >              > Please find a new webrev with the changes at http://cr.openjdk.java.net/~lucy/webrevs/8207343.01
        >          >      >              >
        >          >      >              > Best Regards,
        >          >      >              > Lutz
        >          >      >              >
        >          >      >              > On 02.08.18, 04:45, "Zhongwei Yao" <Zhongwei.Yao at arm.com> wrote:
        >          >      >              >
        >          >      >              >      Hi, Lutz,
        >          >      >              >
        >          >      >              >      I have tested it on aarch64 by running jtreg tests. And find two tiny issues in vtableStubs_aarch64.cpp's VtableStubs::create_itable_stub function:
        >          >      >              >      1. typecheckSize on line 212 is not defined.
        >          >      >              >      2. estimate on line 211 is not large enough, I get 148 in gc/g1/TestFromCardCacheIndex.java case. Here is the assertion failure from that case:
        >          >      >              >          assert(slop_delta >= 0) failed: itable #3: Code size estimate (140) for lookup_interface_method too small, required: 148
        >          >      >              >
        >          >      >              >      However, I don't have tested the modification in vtableStubs_arm.cpp due to I don't have an arm32 environment at hand.
        >          >      >              >
        >          >      >              >      --
        >          >      >              >      Best regards,
        >          >      >              >      Zhongwei
        >          >      >              >
        >          >      >              >      ________________________________________
        >          >      >              >      From: hotspot-compiler-dev <hotspot-compiler-dev-bounces at openjdk.java.net> on behalf of Schmidt, Lutz <lutz.schmidt at sap.com>
        >          >      >              >      Sent: Monday, July 30, 2018 3:57:05 PM
        >          >      >              >      To: hotspot-compiler-dev at openjdk.java.net
        >          >      >              >      Subject: RFR(M): 8207343: Automate vtable/itable stub size calculation
        >          >      >              >
        >          >      >              >      Dear all,
        >          >      >              >
        >          >      >              >      may I please request reviews for this change:
        >          >      >              >
        >          >      >              >      Bug:    https://bugs.openjdk.java.net/browse/JDK-8207343
        >          >      >              >      Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8207343.00/
        >          >      >              >
        >          >      >              >      With this change, I try to get rid of the a-priory size guessing for vtable and itable stubs. Please refer to the bug description for all the details. I didn't want to duplicate that text.
        >          >      >              >
        >          >      >              >      ARM and AARCH64 help requested!
        >          >      >              >      The edits in vtableStubs_aarch64.cpp and vtableStubs_arm.cpp are made blindfolded. I am neither an ARM expert nor do I have build or test hardware available. I would be very grateful if one of the ARM gurus could please fill in for me.
        >          >      >              >
        >          >      >              >      Thank you!
        >          >      >              >      Lutz
        >          >      >              >
        >          >      >              >
        >          >      >              >
        >          >      >              >
        >          >      >
        >          >      >
        >          >      >
        >          >      >
        >          >      >
        >          >      >
        >          >
        >          >
        >          
        >      
        >      
        > 
        
    
    

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ULTransition.log
Type: application/octet-stream
Size: 2532 bytes
Desc: ULTransition.log
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20180816/4314c5e4/ULTransition.log>


More information about the hotspot-compiler-dev mailing list