Thank you for the review! Comments inline..
> On May 4, 2020, at 1:28 AM, Stefan Karlsson <stefan.karls...@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/
