RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Mikael Vidstedt]

Thank you for the review! Comments inline..

> On May 4, 2020, at 1:28 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Hi Mikael,
> 
> On 2020-05-04 07:12, Mikael Vidstedt wrote:
>> Please review this change which implements part of JEP 381:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>> webrev: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
> 
> I went over this patch and collected some comments:
> 
> src/hotspot/share/adlc/output_c.cpp
> src/hotspot/share/adlc/output_h.cpp
> 
> Awkward code layout after change to.

Indeed - fixed!

> src/hotspot/share/c1/c1_Runtime1.cpp
> src/hotspot/share/classfile/classListParser.cpp
> src/hotspot/share/memory/arena.hpp
> src/hotspot/share/opto/chaitin.cpp
> test/hotspot/jtreg/gc/TestCardTablePageCommits.java
> 
> Surrounding comments still refers to Sparc and/or Solaris.
> 
> There are even more places if you search in the rest of the HotSpot source. Are we leaving those for a separate cleanup pass?

Correct - I deliberately avoided changing comments that were not immediately “obvious” how to address and/or that were pre-existing issues, since it’s not necessarily wrong for a comment to refer to Solaris or SPARC even after these changes. I would prefer to do that as follow-ups. Fair?

> src/hotspot/share/gc/g1/g1HeapRegionAttr.hpp
> 
> Remove comment:
>  // We use different types to represent the state value depending on platform as
>  // some have issues loading parts of words.

Fixed.

> src/hotspot/share/gc/shared/memset_with_concurrent_readers.hpp
> 
> Fuse the declaration and definition, now that we only have one implementation. Maybe even remove function/file at some point.

Fixed (fused).

> src/hotspot/share/utilities/globalDefinitions.hpp
> 
> Now that STACK_BIAS is always 0, should we remove its usages? Follow-up RFE?

Yes, this is one of the things I have on my list to file a follow-up for.

> src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
> 
> Maybe remove decryptSuffix?

Fixed.

> src/utils/hsdis/Makefile
> 
> Is this really correct?
> 
> Shouldn't:
> ARCH1=$(CPU:x86_64=amd64)
> ARCH2=$(ARCH1:i686=i386)
> ARCH=$(ARCH2:sparc64=sparcv9)
> 
> be changed to:
> ARCH1=$(CPU:x86_64=amd64)
> ARCH=$(ARCH1:i686=i386)
> 
> so that we have ARCH defined?

Very good catch! This Makefile could use some indentation love or just a plain rewrite.. In either case I fixed the ARCH definition and tested it to make sure the end result seemed to do the right thing (and AFAICT it does).

> Other than that this looks good to me.

Thank you!

Cheers,
Mikael

> 
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>> Note: When reviewing this, please be aware that this exercise was *extremely* mind-numbing, so I appreciate your help reviewing all the individual changes carefully. You may want to get that coffee cup filled up (or whatever keeps you awake)!
>> Background:
>> Because of the size of the total patch and wide range of areas touched, this patch is one out of in total six partial patches which together make up the necessary changes to remove the Solaris and SPARC ports. The other patches are being sent out for review to mailing lists appropriate for the respective areas the touch. An email will be sent to jdk-dev summarizing all the patches/reviews. To be clear: this patch is *not* in itself complete and stand-alone - all of the (six) patches are needed to form a complete patch. Some changes in this patch may look wrong or incomplete unless also looking at the corresponding changes in other areas.
>> For convenience, I’m including a link below[1] to the full webrev, but in case you have comments on changes in other areas, outside of the files included in this thread, please provide those comments directly in the thread on the appropriate mailing list for that area if possible.
>> In case it helps, the changes were effectively produced by searching for and updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. More information about the areas impacted can be found in the JEP itself.
>> A big thank you to Igor Ignatyev for helping make the changes to the hotspot tests!
>> Also, I have a short list of follow-ups which I’m going to look at separately from this JEP/patch, mainly related to command line options/flags which are no longer relevant and should be deprecated/obsoleted/removed.
>> Testing:
>> A slightly earlier version of this change successfully passed tier1-8, as well as client tier1-2. Additional testing will be done after the first round of reviews has been completed.
>> Cheers,
>> Mikael
>> [1] http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/