Re: RFR: 8333566: Remove unused methods
On Tue, 4 Jun 2024 20:51:52 GMT, Cesar Soares Lucas wrote:
> Please, consider this patch to remove unused methods from the code base. To
> the best of my knowledge, these methods are only defined but never used.
>
> Here is a list with names of delete methods:
> https://gist.github.com/JohnTortugo/fccc29781a1b584c03162aa4e160e874
>
> Tested with Linux x86_64 tier1-4, GHA, and only cross building to other
> platforms.
src/hotspot/cpu/s390/vm_version_s390.hpp line 516:
> 514: static void set_has_CompareTrap() { _features[0] |=
> GnrlInstrExtFacilityMask; }
> 515: static void set_has_RelativeLoadStore() { _features[0] |=
> GnrlInstrExtFacilityMask; }
> 516: static void set_has_ProcessorAssist() { _features[0] |=
> ProcessorAssistMask; }
This looks incorrect; there exist a second definition below;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19550#discussion_r1630102121
Re: RFR: 8333566: Remove unused methods
On Tue, 4 Jun 2024 20:51:52 GMT, Cesar Soares Lucas wrote:
> Please, consider this patch to remove unused methods from the code base. To
> the best of my knowledge, these methods are only defined but never used.
>
> Here is a list with names of delete methods:
> https://gist.github.com/JohnTortugo/fccc29781a1b584c03162aa4e160e874
>
> Tested with Linux x86_64 tier1-4, GHA, and only cross building to other
> platforms.
src/hotspot/cpu/s390/vm_version_s390.hpp line 516:
> 514: static void set_has_CompareTrap() { _features[0] |=
> GnrlInstrExtFacilityMask; }
> 515: static void set_has_RelativeLoadStore() { _features[0] |=
> GnrlInstrExtFacilityMask; }
> 516: static void set_has_GnrlInstrExtensions() { _features[0] |=
> GnrlInstrExtFacilityMask; }
I know this PR is still in draft state. Just a thought: I would like to keep
the methods in `vm_version_s390.hpp` file for now. I'm planning to remove the
checks applicable to older hardware. So it would be better, If I clean these
methods as a part of that PR :-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19550#discussion_r1628627936
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 01:13:20 GMT, SendaoYan wrote: >> Hi all, >> ObjectMonitorUsage.java failed with `unexpected waiter_count` after >> [JDK-8328083](https://bugs.openjdk.org/browse/JDK-8328083) on linux x86_32. >> There are two changes in this PR: >> 1. In `JvmtiEnvBase::get_object_monitor_usage` function, change from >> `java_lang_VirtualThread::is_instance(thread_oop)` to >> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the >> alternative implementation. >> 2. In `Threads::get_pending_threads(ThreadsList *, int, address)` function >> of threads.cpp file, change from >> `java_lang_VirtualThread::is_instance(thread_oop)` to >> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the >> alternative implementation. This modified function only used by >> `JvmtiEnvBase::get_object_monitor_usage(JavaThread*, jobject, >> jvmtiMonitorUsage*)`, so the risk of the modified on threads.cpp file is low. >> >> >> >> Additional testing: >> - [x] linux x86_32 run all testcases in serviceability/jvmti, all testcases >> run successed expect >> `serviceability/jvmti/vthread/GetThreadState/GetThreadStateTest.java#default` >> run failed. This test also run failed before this PR, which has been >> recorded in [JDK-8333140](https://bugs.openjdk.org/browse/JDK-8333140) >> - [x] linux x86_64 run all testcases in serviceability/jvmti, all testcases >> run successed. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > change from java_lang_VirtualThread::is_instance(thread_oop) to > hread_oop->is_a(vmClasses::BaseVirtualThread_klass()) in > Threads::get_pending_threads() I have tested it on s390x as well. I don't see any new test failure appearing. Thanks for fixing it. - Marked as reviewed by amitkumar (Committer). PR Review: https://git.openjdk.org/jdk/pull/19405#pullrequestreview-2092069640
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes
On Fri, 29 Mar 2024 19:35:45 GMT, Vladimir Kozlov wrote:
> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE
> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365)
> which was used for AOT [JEP 295](https://openjdk.org/jeps/295)
> implementation in JDK 9. The code was left in HotSpot assuming it will help
> in a future. But during work on Leyden we decided to not use it. In Leyden
> cached compiled code will be restored in CodeCache as normal nmethods: no
> need to change VM's runtime and GC code to process them.
>
> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce
> header size in separate changes. In these changes I did simple fields
> reordering to keep small (1 byte) fields together.
>
> I do not see (and not expected) performance difference with these changes.
>
> Tested tier1-5, xcomp, stress. Running performance testing.
>
> I need help with testing on platforms which Oracle does not support.
I performed the build + testing `{fastdebug, release, slowdebug} X {tier1}` on
`s390x` and result looks fine.
-
PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2029655163
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v22]
On Mon, 12 Feb 2024 18:04:21 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Remove not matched trailing whitespaces Maybe the copyright year could be updated. Nothing else on my side. - Marked as reviewed by amitkumar (Committer). PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1881747083
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v2]
On Thu, 23 Nov 2023 08:08:51 GMT, suchismith1993 wrote: > The JBS issue with respect to that has been closed. Need to check if that PR > is required. Currently putting it on hold. This response on the issue suggest otherwise: : The JDK does not support dynamically loaded archive files (.a files) and there are no plans to add this support. Closing as will not fix - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1824009424
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v2]
On Wed, 22 Nov 2023 16:24:24 GMT, suchismith1993 wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > suchismith1993 has updated the pull request incrementally with one additional > commit since the last revision: > > change macro position Also are you planning to close this one : https://github.com/openjdk/jdk/pull/16490 ? JBS issue is already closed. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1823849323
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v2]
On Wed, 22 Nov 2023 16:24:24 GMT, suchismith1993 wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> suchismith1993 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change macro position
some nits you might want to consider.
src/hotspot/os/aix/os_aix.cpp line 3064:
> 3062:
> 3063: //Replaces provided path with alternate path for the given file,if it
> doesnt exist.
> 3064: //For AIX,this replaces .so with .a.
Suggestion:
// Replaces the specified path with an alternative path for the given file if
the original path doesn't exist.
// For AIX, this replaces extension from ".so" to ".a".
src/hotspot/os/aix/os_aix.cpp line 3065:
> 3063: //Replaces provided path with alternate path for the given file,if it
> doesnt exist.
> 3064: //For AIX,this replaces .so with .a.
> 3065: void os::Aix::mapAlternateName(char* buffer, const char *extension) {
Suggestion:
void os::Aix::map_alternate_name(char* buffer, const char *extension) {
src/hotspot/os/aix/os_aix.hpp line 181:
> 179: static int stat64x_via_LIBPATH(const char* path, struct stat64x* stat);
> 180: // Provide alternate path name,if file does not exist.
> 181: static void mapAlternateName(char* buffer, const char *extension);
Suggestion:
// Provides alternate path name, if file does not exist.
static void map_alternate_name(char* buffer, const char *extension);
-
Changes requested by amitkumar (Committer).
PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1745734586
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1402935976
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1402936171
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1402936497
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions Thanks for addressing the comment. Looks Good :-) - Marked as reviewed by amitkumar (Author). PR Review: https://git.openjdk.org/jdk/pull/14198#pullrequestreview-1452183165
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code
On Mon, 29 May 2023 10:09:15 GMT, Johan Sjölen wrote:
> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes
> I'd appreciate if this was considered trivial.
not a review, but would you like to check if these could replaced as well :-)
./cpu/ppc/macroAssembler_ppc.hpp:735: void load_klass_check_null(Register dst,
Register src, Label* is_null = NULL);
./cpu/ppc/stubGenerator_ppc.cpp:4700:if (UnsafeCopyMemory::_table == NULL) {
./cpu/riscv/codeBuffer_riscv.cpp:74: if
(cb->stubs()->maybe_expand_to_ensure_remaining(total_requested_size) &&
cb->blob() == NULL) {
./share/include/jvm.h:423: * Find a class from a boot class loader. Returns
NULL if class not found.
./share/include/cds.h:72: char* _mapped_base; // Actually mapped
address (NULL if this region is not mapped).
./share/opto/runtime.cpp:491: fields[TypeFunc::Parms+0] = NULL; // void
-
PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1567074248
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v15]
On Mon, 17 Apr 2023 20:59:06 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Merge branch 'master' into 8304915-arch-enum > - ArchTest on Debian RISC-V 64 confirmed by reviewer > - Fixed isPPC64(). >Consolidated switch cases in ArchTest. >Moved mapping of build TARGET_OS and TARGET_CPU to the build >to avoid multiple mappings in more than one place. > - Correct mapping and test of ppc64 > - Add ppc64 as mapping to PPC64 Architecture > - Modified test to check Architecture is64bits() and isLittleEndian() >against Unsafe respective values. >Relocated code mapping OS name and arch name from PlatformProps to >OperatingSystem and Architecture. Kept the mapping of names >in the template close to where the values are filled in by the build. > - Remove unused static and import of Stabile > - Rename OperatingSystemProps to PlatformProps. >Refactor OperatingSystem initialization to use strings instead of integers. > - Revised mapping mechanism of build target architecture names to enum > values. >Unrecognized values from the build are mapped to enum value "OTHER". >Renamed PPC64LE to PPC64 to reflect only the architecture, not the > endianness. >Added an `isLittleEndian` method to return the endianness (not currently > used anywhere) > - Revert changes to jdk.accessibility AccessBridge > - ... and 7 more: https://git.openjdk.org/jdk/compare/8858d543...99a93b7e for s390x, build is fine and tier1 (specifically `ArchTest.java`) passes. Thanks for the change. - Marked as reviewed by amitkumar (Author). PR Review: https://git.openjdk.org/jdk/pull/13357#pullrequestreview-1392455297
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v15]
On Wed, 19 Apr 2023 13:22:54 GMT, Martin Doerr wrote: >> Roger Riggs has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 17 commits: >> >> - Merge branch 'master' into 8304915-arch-enum >> - ArchTest on Debian RISC-V 64 confirmed by reviewer >> - Fixed isPPC64(). >>Consolidated switch cases in ArchTest. >>Moved mapping of build TARGET_OS and TARGET_CPU to the build >>to avoid multiple mappings in more than one place. >> - Correct mapping and test of ppc64 >> - Add ppc64 as mapping to PPC64 Architecture >> - Modified test to check Architecture is64bits() and isLittleEndian() >>against Unsafe respective values. >>Relocated code mapping OS name and arch name from PlatformProps to >>OperatingSystem and Architecture. Kept the mapping of names >>in the template close to where the values are filled in by the build. >> - Remove unused static and import of Stabile >> - Rename OperatingSystemProps to PlatformProps. >>Refactor OperatingSystem initialization to use strings instead of >> integers. >> - Revised mapping mechanism of build target architecture names to enum >> values. >>Unrecognized values from the build are mapped to enum value "OTHER". >>Renamed PPC64LE to PPC64 to reflect only the architecture, not the >> endianness. >>Added an `isLittleEndian` method to return the endianness (not currently >> used anywhere) >> - Revert changes to jdk.accessibility AccessBridge >> - ... and 7 more: https://git.openjdk.org/jdk/compare/8858d543...99a93b7e > > test/jdk/jdk/internal/util/ArchTest.java line 71: > >> 69: case "aarch64" -> AARCH64; >> 70: case "riscv64" -> RISCV64; >> 71: case "s390x", "s390" -> S390; // unverified > > This was also verified according to comments. Right, @offamitkumar? Yes, you're correct @TheRealMDoerr - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1171580811
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v10]
On Tue, 11 Apr 2023 18:07:41 GMT, Martin Doerr wrote: > Another remark: Old JDK on s390 used "os.arch = zArch_64", current one > "os.arch = s390x". @offamitkumar: You probably want to take a look. Martin, only concern was that I didn't have a good experience with `s390x` string in [past](https://github.com/openjdk/jdk/pull/13228#issuecomment-1488599607). But, I assume that `default` in `mapArchString` will unintentionally handle that scenario here. I tested it, and 'ArchTest.java' also passes. I guess @RealLucy might have some pointer over this. - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1504542573
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v14]
On Mon, 27 Mar 2023 14:43:04 GMT, Matias Saavedra Silva
wrote:
>> The current structure used to store the resolution information for
>> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its
>> ambigious fields f1 and f2. This structure can hold information for fields,
>> methods, and invokedynamics and each of its fields can hold different types
>> of values depending on the entry.
>>
>> This enhancement proposes a new structure to exclusively contain
>> invokedynamic information in a manner that is easy to interpret and easy to
>> extend. Resolved invokedynamic entries will be stored in an array in the
>> constant pool cache and the operand of the invokedynamic bytecode will be
>> rewritten to be the index into this array.
>>
>> Any areas that previously accessed invokedynamic data from
>> ConstantPoolCacheEntry will be replaced with accesses to this new array and
>> structure. Verified with tier1-9 tests.
>>
>> The PPC was provided by @reinrich and the RISCV port was provided by
>> @DingliZhang and @zifeihan.
>>
>> This change supports the following platforms: x86, aarch64, PPC, and RISCV
>
> Matias Saavedra Silva has updated the pull request incrementally with one
> additional commit since the last revision:
>
> RISCV patch and aarch64 improvement
`{tier1, tier2} X {fast debug, slow debug, release}` testing done for s390x. PR
seems clean.
@matias9927 please include port for s390x from this commit:
https://github.com/offamitkumar/jdk/commit/a582f32f97aefba33cebaf4ace540681dfc0eff5
Thanks
src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp line 652:
> 650: // Scale the index to be the entry index *
> sizeof(ResolvedInvokeDynamicInfo)
> 651: __ sldi(size, size, log2i_exact(sizeof(ResolvedIndyEntry)));
> 652: __ add(cache, cache, size);
@reinrich Is there any specific reason, why you're not calling
load_resolved_indy_entry() method here. On s390x build/changes are stable even
with calling that helper method.
-
Marked as reviewed by amitkumar (Author).
PR Review: https://git.openjdk.org/jdk/pull/12778#pullrequestreview-1361010070
PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1150566670
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v14]
On Mon, 27 Mar 2023 14:43:04 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > RISCV patch and aarch64 improvement Hi Matias, s390x port is almost complete. All builds are successful & tier1 test for fast debug are complete. For other builds, tests are in progress. Please don't integrate, Wait for us Thanks - PR Comment: https://git.openjdk.org/jdk/pull/12778#issuecomment-1486414339
Re: RFR: 8299795: Relativize locals in interpreter frames
On Tue, 17 Jan 2023 13:33:34 GMT, Martin Doerr wrote: >>> Works on PPC64. Thanks! Tests have passed on other platforms as well. >> >> Does "other platforms" include S390? > >> > Works on PPC64. Thanks! Tests have passed on other platforms as well. >> >> Does "other platforms" include S390? > > No, @backwaterred you may want to check. Hi @TheRealMDoerr and @fbredber , I've executed tier1 & tier2 test on slow, fast and release build on s390 and test-result seems fine. But please let me know if any specific tier I need to test. I would happily do it. - PR: https://git.openjdk.org/jdk/pull/11902
