RFR(M): 8195112: x86 (32 bit): implementation for Thread-local handshakes

[Lindenmaier, Goetz]

Hi Marting, 

thanks for fixing the issues. Looks good.

Best regards,
  Goetz.

> -----Original Message-----
> From: Doerr, Martin
> Sent: Montag, 19. Februar 2018 19:12
> To: Robbin Ehn <robbin.ehn at oracle.com>; hotspot-runtime-
> dev at openjdk.java.net
> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Subject: RE: RFR(M): 8195112: x86 (32 bit): implementation for Thread-local
> handshakes
> 
> Hi Robbin and Götz,
> 
> thank you very much for reviewing.
> 
> @Robbin:
> > Maybe add an assert here for 64-bit that it really is r15.
> Assertion added.
> 
> @Götz:
> > c1_LIRAssembler_x86.cpp:
> > Address(poll_addr, in_bytes(Thread::polling_page_offset()
> Done.
> 
> > macroAssembler_x86.hpp
> > Maybe you could coment above the definition of safepoint_poo()
> Done.
> 
> > sharedRuntime_x86_32.cpp:
> > Please break comment to shorter lines.
> The comments are an exact copy of the 64 bit version. I want to keep them
> consistent.
> 
> > Sometimes, you protect code by _LP64, sometimes by AMD64.
> Good point. I've changed some of them to be consistent with the
> surrounding code.
> Unfortunately, this is not totally consistent. My observation is that:
> - x86 platform code seems to prefer _LP64
> - exception: nativeInst_x86 uses AMD64 in the surrounding code so I've kept
> this there
> - shared code needs to use exact platform defines, of course, therefore IA32
> in arguments.cpp
> 
> New webrev is here:
> http://cr.openjdk.java.net/~mdoerr/8195112_x86_local_poll/webrev.02/
> 
> I'll test it in our nightly tests and then try to get it through the submission
> forest.
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Robbin Ehn [mailto:robbin.ehn at oracle.com]
> Sent: Montag, 19. Februar 2018 17:58
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-
> dev at openjdk.java.net
> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Subject: Re: RFR(M): 8195112: x86 (32 bit): implementation for Thread-local
> handshakes
> 
> Hi Martin,
> 
> On 02/19/2018 03:54 PM, Lindenmaier, Goetz wrote:
> > Hi Martin,
> >
> > I had a look at your change.
> >
> > c1_LIRAssembler_x86.cpp:
> > Address(poll_addr, in_bytes(Thread::polling_page_offset()
> > Why do you need to use in_bytes to cast away ByteSize?
> > In the debug build, there is a proper Address constructor, and
> > in opt it should be pointless.  In other places, you don't use it
> > either.
> >
> > macroAssembler_x86.hpp
> > Maybe you could coment above the definition of safepoint_poo()
> 
> Maybe add an assert here for 64-bit that it really is r15.
> 
> > // If thread_reg is != noreg the code assumes the register passed contains
> > // the thread.
> >
> > sharedRuntime_x86_32.cpp:
> > Please break comment to shorter lines.
> >
> > Sometimes, you protect code by _LP64, sometimes by AMD64.
> > Why? (I prefer AMD64 to protect code that depends on the architectural
> > extension for 64-bit ...).
> > Instead of ifndef AMD64 you can use ifdef IA32 which you do
> > at other places :)
> >
> > I think the submission forest is good to show you didn't break
> > X86_64. So I think if it passes there you can push it.
> 
> Yes that would be good.
> 
> >
> > Reviewed.
> 
> +1
> 
> ->
> 
> >
> > Best regards,
> >    Goetz.
> >
> > -----Original Message-----
> > From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of Doerr, Martin
> > Sent: Monday, February 19, 2018 1:15 PM
> > To: Robbin Ehn <robbin.ehn at oracle.com>; hotspot-runtime-
> dev at openjdk.java.net
> > Subject: [CAUTION] RE: RFR(M): 8195112: x86 (32 bit): implementation for
> Thread-local handshakes
> >
> > Hi,
> >
> > I have disabled ThreadLocalHandshakes by default on linux 32 bit.
> > After some performance measurements, I have also switched it off on
> Windows 32 bit if the server compiler is not active (arguments.cpp) which is
> the default.
> > C1 performance is affected if enabled. C2 and tiered performance look ok.
> >
> > So on 32 bit, ThreadLocalHandshakes are only used on Windows when
> enabling server mode with this updated version.
> >
> > Please review:
> > http://cr.openjdk.java.net/~mdoerr/8195112_x86_local_poll/webrev.01/
> >
> > @Robbin: I guess the submission forest is not helpful for 32 bit?
> 
> Unfortunately no.
> 
> > Will I still need a sponsor for it?
> 
> I don't know.
> 
> Thanks for fixing, Robbin
> 
> >
> > Best regards,
> > Martin
> >
> >
> > -----Original Message-----
> > From: Andrew Haley [mailto:aph at redhat.com]
> > Sent: Donnerstag, 8. Februar 2018 17:28
> > To: Doerr, Martin <martin.doerr at sap.com>; David Holmes
> <david.holmes at oracle.com>; Robbin Ehn <robbin.ehn at oracle.com>;
> hotspot-runtime-dev at openjdk.java.net
> > Subject: Re: RFR(M): 8195112: x86 (32 bit): implementation for Thread-local
> handshakes
> >
> > On 08/02/18 13:04, Doerr, Martin wrote:
> >> Are you planning to fix it soon?
> >> If so, we can delay my change until it was pushed.
> >
> > I was, but I keep getting interrupted by urgent things.
> >
> >> Otherwise, we can keep ThreadLocalHandshakes off by default on linux
> x86 32 bit.
> >> That would be ok for us, too. We'd just like the feature to be available.
> >
> > Sure, do that.  Thanks.
> >