Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
> On May 8, 2020, at 12:48 PM, Daniel D. Daugherty
> wrote:
>
> On 5/7/20 1:35 AM, Mikael Vidstedt wrote:
>> New webrev here:
>>
>> webrev:
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/
>
> This pretty much says it all:
>
> > Summary of changes:90904 lines changed: 8 ins; 90725 del; 171 mod;
> > 103780 unchg
>
>
> My review is focused on looking at the changes and not looking for missed
> changes. I figure there's enough work here just looking at the changes to
> keep me occupied for a while and enough people have posted comments about
> finding other things to be examined, etc...
>
> Unlike my normal reviews, I won't be listing all the touched files;
> (there's _only_ 427 of them...)
>
> Don't forget to make a copyright year update pass before you push.
Yup - I have added it in 10 different places on my TODO list to maximize the
likelihood of me remembering it :)
>
> src/hotspot/os/posix/os_posix.hpp
> L174
> old L175 #ifndef SOLARIS
> L176
>nit - on most of this style of deletion you also got rid of
>one of the blank lines, but not here.
Oops, will fix.
> src/hotspot/share/utilities/dtrace.hpp
> old L42: #elif defined(__APPLE__)
> old L44: #include
> old L45: #else
> new L32: #include
> was previous included only for __APPLE__ and it
> is now there for every platform. Any particular reason?
No particular reason other than "it looks cleaner". I guess we could see if the
include can be removed altogether.
> Thumbs up!
Thanks for the review!!
Cheers,
Mikael
>
>> incremental:
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot.incr/open/webrev/
>>
>> Remaining items:
>>
>> * File follow-up to remove STACK_BIAS
>>
>> * File follow-ups to change/update/remove flags and/or flag documentation:
>> UseLWPSynchronization, BranchOnRegister, LIRFillDelaySlots,
>> ArrayAllocatorMallocLimit, ThreadPriorityPolicy
>>
>> * File follow-up(s) to update comments ("solaris", “sparc”, “solstudio”,
>> “sunos”, “sun studio”, “s compiler bug”, “niagara”, …)
>>
>>
>> Please let me know if there’s something I have missed!
>>
>> Cheers,
>> Mikael
>>
>>> On May 3, 2020, at 10:12 PM, 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/
>>> 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/
>>>
>
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
On 5/7/20 1:35 AM, Mikael Vidstedt wrote:
New webrev here:
webrev:
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/
This pretty much says it all:
> Summary of changes: 90904 lines changed: 8 ins; 90725 del; 171
mod; 103780 unchg
My review is focused on looking at the changes and not looking for missed
changes. I figure there's enough work here just looking at the changes to
keep me occupied for a while and enough people have posted comments about
finding other things to be examined, etc...
Unlike my normal reviews, I won't be listing all the touched files;
(there's _only_ 427 of them...)
Don't forget to make a copyright year update pass before you push.
src/hotspot/os/posix/os_posix.hpp
L174
old L175 #ifndef SOLARIS
L176
nit - on most of this style of deletion you also got rid of
one of the blank lines, but not here.
src/hotspot/share/utilities/dtrace.hpp
old L42: #elif defined(__APPLE__)
old L44: #include
old L45: #else
new L32: #include
was previous included only for __APPLE__ and it
is now there for every platform. Any particular reason?
Thumbs up!
Dan
incremental:
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot.incr/open/webrev/
Remaining items:
* File follow-up to remove STACK_BIAS
* File follow-ups to change/update/remove flags and/or flag documentation:
UseLWPSynchronization, BranchOnRegister, LIRFillDelaySlots,
ArrayAllocatorMallocLimit, ThreadPriorityPolicy
* File follow-up(s) to update comments ("solaris", “sparc”, “solstudio”,
“sunos”, “sun studio”, “s compiler bug”, “niagara”, …)
Please let me know if there’s something I have missed!
Cheers,
Mikael
On May 3, 2020, at 10:12 PM, 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/
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/
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
Hi,
On 07.05.20 07:35, Mikael Vidstedt wrote:
New webrev here:
webrev:
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/
incremental:
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot.incr/open/webrev/
Remaining items:
* File follow-up to remove STACK_BIAS
* File follow-ups to change/update/remove flags and/or flag documentation:
UseLWPSynchronization, BranchOnRegister, LIRFillDelaySlots,
ArrayAllocatorMallocLimit, ThreadPriorityPolicy
* File follow-up(s) to update comments ("solaris", “sparc”, “solstudio”,
“sunos”, “sun studio”, “s compiler bug”, “niagara”, …)
Please let me know if there’s something I have missed!
Looks good.
Thomas
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
Hi Mikael, yes, Vladimir's reply made it clear, let's hope all the needed changes got upstream before the next graal update so it goes smoothly. Cheers, -- Igor > On May 6, 2020, at 10:27 PM, Mikael Vidstedt > wrote: > > > Igor, thank you for the review, and again for helping make the test changes > in the first place! :) > > I hope Vladimir’s reply clarifies how we’re planning on handling the Graal > related changes. > > Cheers, > Mikael > >> On May 4, 2020, at 2:29 PM, Igor Ignatyev wrote: >> >> Hi Mikael, >> >> the changes in /test/ look good to me. >> >> I have a question regarding src/jdk.internal.vm.compiler/*, aren't these >> files part of graal-compiler and hence will be brought back by the next >> graal update? >> >> Thanks, >> -- Igor >> >>> On May 3, 2020, at 10:12 PM, 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/ >>> 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/ >>> >> >
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
> On May 7, 2020, at 1:35 AM, Mikael Vidstedt
> wrote:
>
>
> New webrev here:
>
> webrev:
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/
> incremental:
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot.incr/open/webrev/
>
> Remaining items:
>
> * File follow-up to remove STACK_BIAS
>
> * File follow-ups to change/update/remove flags and/or flag documentation:
> UseLWPSynchronization, BranchOnRegister, LIRFillDelaySlots,
> ArrayAllocatorMallocLimit, ThreadPriorityPolicy
>
> * File follow-up(s) to update comments ("solaris", “sparc”, “solstudio”,
> “sunos”, “sun studio”, “s compiler bug”, “niagara”, …)
>
>
> Please let me know if there’s something I have missed!
Looks good.
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
New webrev here:
webrev:
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/
incremental:
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot.incr/open/webrev/
Remaining items:
* File follow-up to remove STACK_BIAS
* File follow-ups to change/update/remove flags and/or flag documentation:
UseLWPSynchronization, BranchOnRegister, LIRFillDelaySlots,
ArrayAllocatorMallocLimit, ThreadPriorityPolicy
* File follow-up(s) to update comments ("solaris", “sparc”, “solstudio”,
“sunos”, “sun studio”, “s compiler bug”, “niagara”, …)
Please let me know if there’s something I have missed!
Cheers,
Mikael
> On May 3, 2020, at 10:12 PM, 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/
> 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/
>
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
Igor, thank you for the review, and again for helping make the test changes in the first place! :) I hope Vladimir’s reply clarifies how we’re planning on handling the Graal related changes. Cheers, Mikael > On May 4, 2020, at 2:29 PM, Igor Ignatyev wrote: > > Hi Mikael, > > the changes in /test/ look good to me. > > I have a question regarding src/jdk.internal.vm.compiler/*, aren't these > files part of graal-compiler and hence will be brought back by the next graal > update? > > Thanks, > -- Igor > >> On May 3, 2020, at 10:12 PM, 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/ >> 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/ >> >
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
Vladimir, thank you for the review! Note that based on Stefan’s comments I have removed the decryptSuffix variable in src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java in the upcoming webrev. Cheers, Mikael > On May 4, 2020, at 12:01 PM, Vladimir Kozlov > wrote: > > JIT, AOT, JVMCI and Graal changes seem fine to me. > > It would be interesting to see shared code execution coverage change. There > are places where we use flags and setting instead of #ifdef SPARC which may > not be executed now or executed partially. We may simplify such code too. > > Thanks, > Vladimir > > On 5/3/20 10:12 PM, 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/ >> 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/
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
Kim, thank you for the review! Comments inline..
> On May 4, 2020, at 3:47 AM, Kim Barrett wrote:
>
>> On May 4, 2020, at 1:12 AM, 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/
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>
> I've only looked at the src/hotspot changes so far. I've not
> duplicated comments already made by Stefan.
>
> Looks good, other than a few very minor issues, some of which might
> already be covered by planned followup RFEs.
>
> --
>
> I think with sparc removal, c1's pack64/unpack64 stuff is no longer
> used. So I think that can be removed from c1_LIR.[ch]pp too.
Good catch. Fixed.
> --
> src/hotspot/share/opto/generateOptoStub.cpp
> 225 // Clear last_Java_pc and (optionally)_flags
>
> The sparc-specific clearing of "flags" is gone.
Fixed.
> --
> src/hotspot/share/runtime/deoptimization.cpp
> 1086 *((jlong *) check_alignment_get_addr(obj, index, 8)) = (jlong)
> *((jlong *) &val);
>
> [pre-existing]
> The rhs cast to jlong is unnecessary, since it's dereferencing a jlong*.
When I first updated the code I actually remove the cast, but it just ended up
looking asymmetrical so I chose to leave it there. Let me know if you feel
strongly that it should go. (I don’t like these casts in general).
> --
> src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp
> 236 JVMFlag::Error CompilerThreadPriorityConstraintFunc(intx value, bool
> verbose) {
> 237 return JVMFlag::SUCCESS;
> 238 }
>
> After SOLARIS code removal we no longer need this constraint function.
Fixed. (I had that on my follow-up list, but included it in the upcoming
webrev.)
> --
> src/hotspot/share/runtime/globals.hpp
> 2392 experimental(size_t, ArrayAllocatorMallocLimit,
>\
> 2393 (size_t)-1,
>\
>
> Combine these lines.
Fixed.
> --
> src/hotspot/share/utilities/dtrace.hpp
>
> Shuold just eliminate all traces of HS_DTRACE_WORKAROUND_TAIL_CALL_BUG.
Fixed - more code removed!
Cheers,
Mikael
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
> On May 4, 2020, at 2:11 AM, Thomas Schatzl wrote: > > Hi, > > On 04.05.20 10:28, Stefan Karlsson 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. >> 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.jav > > > 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? > > In addition to "sparc", "solaris", also "solstudio"/"Sun Studio"/"SS compiler > bug"/"niagara" yield some search (=grep) results. > > Some of these locations look like additional RFEs. Ah good! I found and fixed a few additional places based on those strings, but would like to handling the remaining comment updates as RFEs. Thank you for having a look! Cheers, Mikael
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
Thank you for the review! Comments inline.. > On May 4, 2020, at 1:28 AM, Stefan Karlsson > 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/
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
I filed Graal issue to change mx script to filter out SPARC code when we do sync Graal changes into JDK. For Graal shared code we may need to have versioning for latest JDK as we do in other cases. Regards, Vladimir On 5/4/20 2:29 PM, Igor Ignatyev wrote: Hi Mikael, the changes in /test/ look good to me. I have a question regarding src/jdk.internal.vm.compiler/*, aren't these files part of graal-compiler and hence will be brought back by the next graal update? Thanks, -- Igor On May 3, 2020, at 10:12 PM, 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/ 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/
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
Hi Mikael, the changes in /test/ look good to me. I have a question regarding src/jdk.internal.vm.compiler/*, aren't these files part of graal-compiler and hence will be brought back by the next graal update? Thanks, -- Igor > On May 3, 2020, at 10:12 PM, 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/ > 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/ >
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
JIT, AOT, JVMCI and Graal changes seem fine to me. It would be interesting to see shared code execution coverage change. There are places where we use flags and setting instead of #ifdef SPARC which may not be executed now or executed partially. We may simplify such code too. Thanks, Vladimir On 5/3/20 10:12 PM, 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/ 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/
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
> On May 4, 2020, at 1:12 AM, 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/
> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
I've only looked at the src/hotspot changes so far. I've not
duplicated comments already made by Stefan.
Looks good, other than a few very minor issues, some of which might
already be covered by planned followup RFEs.
--
I think with sparc removal, c1's pack64/unpack64 stuff is no longer
used. So I think that can be removed from c1_LIR.[ch]pp too.
--
src/hotspot/share/opto/generateOptoStub.cpp
225 // Clear last_Java_pc and (optionally)_flags
The sparc-specific clearing of "flags" is gone.
--
src/hotspot/share/runtime/deoptimization.cpp
1086 *((jlong *) check_alignment_get_addr(obj, index, 8)) = (jlong)
*((jlong *) &val);
[pre-existing]
The rhs cast to jlong is unnecessary, since it's dereferencing a jlong*.
--
src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp
236 JVMFlag::Error CompilerThreadPriorityConstraintFunc(intx value, bool
verbose) {
237 return JVMFlag::SUCCESS;
238 }
After SOLARIS code removal we no longer need this constraint function.
--
src/hotspot/share/runtime/globals.hpp
2392 experimental(size_t, ArrayAllocatorMallocLimit,
\
2393 (size_t)-1,
\
Combine these lines.
--
src/hotspot/share/utilities/dtrace.hpp
Shuold just eliminate all traces of HS_DTRACE_WORKAROUND_TAIL_CALL_BUG.
--
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
Hi, On 04.05.20 10:28, Stefan Karlsson 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. 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.jav > > 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? In addition to "sparc", "solaris", also "solstudio"/"Sun Studio"/"SS compiler bug"/"niagara" yield some search (=grep) results. Some of these locations look like additional RFEs. Thanks, Thomas
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
> On May 4, 2020, at 4:28 AM, Stefan Karlsson > 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/ > > […] > > 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. I think that cleanup is covered by JDK-8142349.
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
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. 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? 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. 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. src/hotspot/share/utilities/globalDefinitions.hpp Now that STACK_BIAS is always 0, should we remove its usages? Follow-up RFE? src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java Maybe remove decryptSuffix? 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? Other than that this looks good to me. StefanK 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/
