Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
On Wed, 20 Dec 2023 21:11:09 GMT, Kim Barrett wrote: >> [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the >> `GrowableArrayCHeap`. This duplicates the current C-Heap allocation >> capability in `GrowableArray`. I now remove that from `GrowableArray` and >> move all usages to `GrowableArrayCHeap`. >> >> This has a few advantages: >> - Clear separation between arena (and resource area) allocating array and >> C-heap allocating array. >> - We can prevent assigning / copying between arrays of different allocation >> strategies already at compile time, and not only with asserts at runtime. >> - We should not have multiple implementations of the same thing (C-Heap >> backed array). >> - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now >> know that C-Heap backed arrays do not get copied unknowingly. >> >> **Bonus** >> We can now restrict `GrowableArray` element type `E` to be >> `std::is_trivially_destructible::value == true`. The idea is that arena / >> resource allocated arrays get abandoned, often without being even cleared. >> Hence, the elements in the array are never destructed. But if we only use >> elements that are trivially destructible, then it makes no difference if the >> destructors are ever called, or the elements simply abandoned. >> >> For `GrowableArrayCHeap`, we expect that the user eventually calls the >> destructor for the array, which in turn calls the destructors of the >> remaining elements. Hence, it is up to the user to ensure the cleanup. And >> so we can allow non-trivial destructors. >> >> **Testing** >> Tier1-3 + stress testing: pending > > pre-existing: There are a lot of non-static class data members that are > pointers to > GrowableArray that seem like they would be better as direct, e.g. > non-pointers. > > pre-existing: There are a lot of iterations over GrowableArray's that would be > simplified by using range-based-for. > > I'm not a fan of the additional clutter in APIs that the static memory types > add. > If we had a variant of GrowableArrayCHeap that was not itself dynamically > allocatable > and took a memory type to use internally as a constructor argument, then I > think a > lot of that clutter could be eliminated. It could be used for ordinary data > members > that are direct GAs rather than pointers to GAs. I think there is a way to > do something > similar for static data members that are pointers that are dynamically > allocated later, > though that probably requires more work. > > I've not yet reviewed the changes to growableArray.[ch]pp yet, nor the test > changes. > But I've run out of time and energy for this for today. @kimbarrett Thanks for looking at the PR! I see you address a lot of "pre-existing" issues. And you would like GrowableArrayCHeap not have the MEMFLAGS in the template argument but maybe as a constructor argument instead. Or maybe a GACH version that only allocates once, though I guess that would limit what kinds of methods you could call on it... Can we address these issues as separate RFE's? - PR Comment: https://git.openjdk.org/jdk/pull/17160#issuecomment-1865639695
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
On Wed, 20 Dec 2023 14:53:06 GMT, Joachim Kern wrote:
>> On AIX, repeated calls to dlopen referring to the same shared library may
>> result in different, unique dl handles to be returned from libc. In that it
>> differs from typical libc implementations that cache dl handles.
>>
>> This causes problems in the JVM with code that assumes equality of handles.
>> One such problem is in the JVMTI agent handler. That problem was fixed with
>> a local fix to said handler
>> ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this
>> fix causes follow-up problems since it assumes that the file name passed to
>> `os::dll_load()` is the file that has been opened. It prevents efficient,
>> os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality.
>> See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it
>> is a hack that causes other, more uglier hacks to follow (see discussion of
>> https://github.com/openjdk/jdk/pull/16604).
>>
>> We propose a different, cleaner way of handling this:
>>
>> - Handle this entirely inside the AIX versions of os::dll_load and
>> os::dll_unload.
>> - Cache dl handles; repeated opening of a library should return the cached
>> handle.
>> - Increase handle-local ref counter on open, Decrease it on close
>> - Make sure calls to os::dll_load are matched to os::dll_unload (See
>> [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)).
>>
>> This way we mimic dl handle equality as it is implemented on other
>> platforms, and this works for all callers of os::dll_load.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> improve error handling
still ok, small nit inside
src/hotspot/os/aix/porting_aix.cpp line 1033:
> 1031: // filled by os::dll_load(). This way we mimic dl handle equality for a
> library
> 1032: // opened a second time, as it is implemented on other platforms.
> 1033: void* Aix_dlopen(const char* filename, int Flags, const char**
> error_report) {
add assert for error_report != nullptr
-
Marked as reviewed by stuefe (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16920#pullrequestreview-1792301031
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433606032
Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
On Wed, 20 Dec 2023 20:35:05 GMT, Kim Barrett wrote: >> [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the >> `GrowableArrayCHeap`. This duplicates the current C-Heap allocation >> capability in `GrowableArray`. I now remove that from `GrowableArray` and >> move all usages to `GrowableArrayCHeap`. >> >> This has a few advantages: >> - Clear separation between arena (and resource area) allocating array and >> C-heap allocating array. >> - We can prevent assigning / copying between arrays of different allocation >> strategies already at compile time, and not only with asserts at runtime. >> - We should not have multiple implementations of the same thing (C-Heap >> backed array). >> - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now >> know that C-Heap backed arrays do not get copied unknowingly. >> >> **Bonus** >> We can now restrict `GrowableArray` element type `E` to be >> `std::is_trivially_destructible::value == true`. The idea is that arena / >> resource allocated arrays get abandoned, often without being even cleared. >> Hence, the elements in the array are never destructed. But if we only use >> elements that are trivially destructible, then it makes no difference if the >> destructors are ever called, or the elements simply abandoned. >> >> For `GrowableArrayCHeap`, we expect that the user eventually calls the >> destructor for the array, which in turn calls the destructors of the >> remaining elements. Hence, it is up to the user to ensure the cleanup. And >> so we can allow non-trivial destructors. >> >> **Testing** >> Tier1-3 + stress testing: pending > > src/hotspot/share/memory/arena.hpp line 209: > >> 207: >> 208: #ifdef ASSERT >> 209: bool Arena_contains(const Arena* arena, const void* ptr); > > This function doesn't seem necessary. Directly calling arena->contains(ptr) > in the one place it's being seems > like it should suffice. @kimbarrett the reason was that I need to call this from the hpp file, and I encountered some circular dependency I did could not resolve. So I needed to move something off to the cpp files. Either I put it in arena.cpp, or in growableArray.cpp. But If I put things off to growableArray.cpp from the GrowableArray class, then it will not easily instantiate the templates, so that is not possible then. Hence I have to put it into arena.cpp - PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433582008
[jdk22] Integrated: 8321565: [REDO] Heap dump does not contain virtual Thread stack references
On Tue, 19 Dec 2023 22:48:28 GMT, Alex Menkov wrote: > Hi all, > > This pull request contains a backport of commit > [cf948548](https://github.com/openjdk/jdk/commit/cf948548c390c42ca63525d41a9d63ff31349c3a) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Alex Menkov on 13 Dec 2023 and > was reviewed by Serguei Spitsyn, Yi Yang and David Holmes. > > Thanks! This pull request has now been integrated. Changeset: fb3cc98d Author:Alex Menkov URL: https://git.openjdk.org/jdk22/commit/fb3cc98da3313e564a7495cc61208dd235e048b1 Stats: 319 lines in 3 files changed: 152 ins; 81 del; 86 mod 8321565: [REDO] Heap dump does not contain virtual Thread stack references Reviewed-by: sspitsyn Backport-of: cf948548c390c42ca63525d41a9d63ff31349c3a - PR: https://git.openjdk.org/jdk22/pull/21
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
On Wed, 20 Dec 2023 14:53:06 GMT, Joachim Kern wrote:
>> On AIX, repeated calls to dlopen referring to the same shared library may
>> result in different, unique dl handles to be returned from libc. In that it
>> differs from typical libc implementations that cache dl handles.
>>
>> This causes problems in the JVM with code that assumes equality of handles.
>> One such problem is in the JVMTI agent handler. That problem was fixed with
>> a local fix to said handler
>> ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this
>> fix causes follow-up problems since it assumes that the file name passed to
>> `os::dll_load()` is the file that has been opened. It prevents efficient,
>> os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality.
>> See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it
>> is a hack that causes other, more uglier hacks to follow (see discussion of
>> https://github.com/openjdk/jdk/pull/16604).
>>
>> We propose a different, cleaner way of handling this:
>>
>> - Handle this entirely inside the AIX versions of os::dll_load and
>> os::dll_unload.
>> - Cache dl handles; repeated opening of a library should return the cached
>> handle.
>> - Increase handle-local ref counter on open, Decrease it on close
>> - Make sure calls to os::dll_load are matched to os::dll_unload (See
>> [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)).
>>
>> This way we mimic dl handle equality as it is implemented on other
>> platforms, and this works for all callers of os::dll_load.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> improve error handling
A pretty complex solution, but I couldn't spot any real bug. Please consider my
suggestions.
src/hotspot/os/aix/porting_aix.cpp line 25:
> 23: */
> 24: // needs to be defined first, so that the implicit loaded xcoff.h header
> defines
> 25: // the right structures to analyze the loader header of 32 and 64 Bit
> executable files
I don't think we support 32 bit executables.
src/hotspot/os/aix/porting_aix.cpp line 916:
> 914: constexpr int max_handletable = 1024;
> 915: static int g_handletable_used = 0;
> 916: static struct handletableentry g_handletable[max_handletable] = {{0, 0,
> 0, 0}};
Wouldn't `ConcurrentHashTable` be a better data structure? It is already used
in hotspot, can grow dynamically and doesn't need linear search.
src/hotspot/os/aix/porting_aix.cpp line 921:
> 919: // If the libpath cannot be retrieved return an empty path
> 920: static const char* rtv_linkedin_libpath() {
> 921: static char buffer[4096];
Maybe define a constant for the buffer size?
src/hotspot/os/aix/porting_aix.cpp line 927:
> 925: // let libpath point to buffer, which then contains a valid libpath
> 926: // or an empty string
> 927: if (libpath) {
`!= nullptr` is common in hotspot.
src/hotspot/os/aix/porting_aix.cpp line 934:
> 932: // to open it
> 933: snprintf(buffer, 100, "/proc/%ld/object/a.out", (long)getpid());
> 934: FILE* f = 0;
Should be nullptr.
src/hotspot/os/aix/porting_aix.cpp line 990:
> 988: }
> 989: ret = (0 == stat64x(combined.base(), stat));
> 990: os::free (path2);
Please remove the extra whitespace.
src/hotspot/os/aix/porting_aix.cpp line 1026:
> 1024:
> 1025: os::free (libpath);
> 1026: os::free (path2);
Same here.
-
PR Review: https://git.openjdk.org/jdk/pull/16920#pullrequestreview-1791807521
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433267331
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433283111
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433273616
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433270399
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433289382
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433290839
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1433291127
[jdk22] RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable
Hi all, This pull request contains a backport of commit [0f8e4e0a](https://github.com/openjdk/jdk/commit/0f8e4e0a81257c678e948c341a241dc0b810494f) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Serguei Spitsyn on 19 Dec 2023 and was reviewed by Leonid Mesnik and Alan Bateman. Thanks! - Commit messages: - Backport 0f8e4e0a81257c678e948c341a241dc0b810494f Changes: https://git.openjdk.org/jdk22/pull/23/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=23&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311218 Stats: 229 lines in 15 files changed: 196 ins; 0 del; 33 mod Patch: https://git.openjdk.org/jdk22/pull/23.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/23/head:pull/23 PR: https://git.openjdk.org/jdk22/pull/23
Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
On Wed, 20 Dec 2023 19:37:52 GMT, Kim Barrett wrote:
>> [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the
>> `GrowableArrayCHeap`. This duplicates the current C-Heap allocation
>> capability in `GrowableArray`. I now remove that from `GrowableArray` and
>> move all usages to `GrowableArrayCHeap`.
>>
>> This has a few advantages:
>> - Clear separation between arena (and resource area) allocating array and
>> C-heap allocating array.
>> - We can prevent assigning / copying between arrays of different allocation
>> strategies already at compile time, and not only with asserts at runtime.
>> - We should not have multiple implementations of the same thing (C-Heap
>> backed array).
>> - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now
>> know that C-Heap backed arrays do not get copied unknowingly.
>>
>> **Bonus**
>> We can now restrict `GrowableArray` element type `E` to be
>> `std::is_trivially_destructible::value == true`. The idea is that arena /
>> resource allocated arrays get abandoned, often without being even cleared.
>> Hence, the elements in the array are never destructed. But if we only use
>> elements that are trivially destructible, then it makes no difference if the
>> destructors are ever called, or the elements simply abandoned.
>>
>> For `GrowableArrayCHeap`, we expect that the user eventually calls the
>> destructor for the array, which in turn calls the destructors of the
>> remaining elements. Hence, it is up to the user to ensure the cleanup. And
>> so we can allow non-trivial destructors.
>>
>> **Testing**
>> Tier1-3 + stress testing: pending
>
> src/hotspot/share/cds/metaspaceShared.cpp line 840:
>
>> 838:
>> 839: #if INCLUDE_CDS_JAVA_HEAP
>> 840: void
>> VM_PopulateDumpSharedSpace::dump_java_heap_objects(GrowableArrayCHeap> mtClassShared>* klasses) {
>
> pre-existing: Perhaps the argument should be const.
pre-existing and can't attach comment to line#50: `int i;` is dead variable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433116120
Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
On Tue, 19 Dec 2023 16:59:05 GMT, Emanuel Peter wrote:
> [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the
> `GrowableArrayCHeap`. This duplicates the current C-Heap allocation
> capability in `GrowableArray`. I now remove that from `GrowableArray` and
> move all usages to `GrowableArrayCHeap`.
>
> This has a few advantages:
> - Clear separation between arena (and resource area) allocating array and
> C-heap allocating array.
> - We can prevent assigning / copying between arrays of different allocation
> strategies already at compile time, and not only with asserts at runtime.
> - We should not have multiple implementations of the same thing (C-Heap
> backed array).
> - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now
> know that C-Heap backed arrays do not get copied unknowingly.
>
> **Bonus**
> We can now restrict `GrowableArray` element type `E` to be
> `std::is_trivially_destructible::value == true`. The idea is that arena /
> resource allocated arrays get abandoned, often without being even cleared.
> Hence, the elements in the array are never destructed. But if we only use
> elements that are trivially destructible, then it makes no difference if the
> destructors are ever called, or the elements simply abandoned.
>
> For `GrowableArrayCHeap`, we expect that the user eventually calls the
> destructor for the array, which in turn calls the destructors of the
> remaining elements. Hence, it is up to the user to ensure the cleanup. And so
> we can allow non-trivial destructors.
>
> **Testing**
> Tier1-3 + stress testing: pending
pre-existing: There are a lot of non-static class data members that are
pointers to
GrowableArray that seem like they would be better as direct, e.g. non-pointers.
pre-existing: There are a lot of iterations over GrowableArray's that would be
simplified by using range-based-for.
I'm not a fan of the additional clutter in APIs that the static memory types
add.
If we had a variant of GrowableArrayCHeap that was not itself dynamically
allocatable
and took a memory type to use internally as a constructor argument, then I
think a
lot of that clutter could be eliminated. It could be used for ordinary data
members
that are direct GAs rather than pointers to GAs. I think there is a way to do
something
similar for static data members that are pointers that are dynamically
allocated later,
though that probably requires more work.
I've not yet reviewed the changes to growableArray.[ch]pp yet, nor the test
changes.
But I've run out of time and energy for this for today.
src/hotspot/share/cds/dumpTimeClassInfo.hpp line 162:
> 160: private:
> 161: template
> 162: static int array_length_or_zero(GrowableArrayCHeap* array)
> {
Argument could be `GrowableArrayView*`, removing the coupling on the memory
type.
Also, pre-existing: the argument should be const.
src/hotspot/share/cds/metaspaceShared.cpp line 441:
> 439:
> 440: void dump_java_heap_objects(GrowableArrayCHeap*
> klasses) NOT_CDS_JAVA_HEAP_RETURN;
> 441: void dump_shared_symbol_table(GrowableArrayView* symbols) {
pre-existing: Perhaps the arguments to these should be const.
src/hotspot/share/cds/metaspaceShared.cpp line 840:
> 838:
> 839: #if INCLUDE_CDS_JAVA_HEAP
> 840: void
> VM_PopulateDumpSharedSpace::dump_java_heap_objects(GrowableArrayCHeap mtClassShared>* klasses) {
pre-existing: Perhaps the argument should be const.
src/hotspot/share/classfile/compactHashtable.cpp line 54:
> 52:
> 53: _num_entries_written = 0;
> 54: _buckets = NEW_C_HEAP_ARRAY(EntryBucket*, _num_buckets, mtSymbol);
pre-existing: It seems like the code could be simpler if the type of _buckets
was
GrowableArrayCHeap.
src/hotspot/share/classfile/javaClasses.cpp line 1824:
> 1822: // Pick minimum length that will cover most cases
> 1823: int init_length = 64;
> 1824: _methods = new GrowableArrayCHeap mtInternal>(init_length);
Consider renaming init_length => init_capacity.
src/hotspot/share/code/codeCache.hpp line 92:
> 90: private:
> 91: // CodeHeaps of the cache
> 92: typedef GrowableArrayCHeap CodeHeapArray;
pre-existing: Consider moving CodeHeapArray to namespace scope and prefer using
it to the long-form.
If not at namespace scope, it could at least be public in this class and used
throughout, including in public
APIs.
src/hotspot/share/memory/arena.hpp line 209:
> 207:
> 208: #ifdef ASSERT
> 209: bool Arena_contains(const Arena* arena, const void* ptr);
This function doesn't seem necessary. Directly calling arena->contains(ptr) in
the one place it's being seems
like it should suffice.
src/hotspot/share/memory/heapInspection.cpp line 282:
> 280: KlassInfoHisto::KlassInfoHisto(KlassInfoTable* cit) :
> 281: _cit(cit) {
> 282: _elements = new GrowableArrayCHeap mtServiceability>(_histo_initial_size);
pre-existing: Why is this initialization separate from the ctor-initializer?
And this looks like an exampl
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 14:15:48 GMT, Alan Bateman wrote: > Update: ignore this I mis-read that it updates the current thread's suspend > value, not the thread's suspend value. Thanks, Alan. I've also got confused with this and even filed a follow up bug. :) Yes, the initial design was the `_is_disable_suspend` is set/modified/accessed on the current thread only. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1433182764
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v8]
> On AIX, repeated calls to dlopen referring to the same shared library may > result in different, unique dl handles to be returned from libc. In that it > differs from typical libc implementations that cache dl handles. > > This causes problems in the JVM with code that assumes equality of handles. > One such problem is in the JVMTI agent handler. That problem was fixed with a > local fix to said handler > ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this > fix causes follow-up problems since it assumes that the file name passed to > `os::dll_load()` is the file that has been opened. It prevents efficient, > os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality. > See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it > is a hack that causes other, more uglier hacks to follow (see discussion of > https://github.com/openjdk/jdk/pull/16604). > > We propose a different, cleaner way of handling this: > > - Handle this entirely inside the AIX versions of os::dll_load and > os::dll_unload. > - Cache dl handles; repeated opening of a library should return the cached > handle. > - Increase handle-local ref counter on open, Decrease it on close > - Make sure calls to os::dll_load are matched to os::dll_unload (See > [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)). > > This way we mimic dl handle equality as it is implemented on other platforms, > and this works for all callers of os::dll_load. Joachim Kern has updated the pull request incrementally with one additional commit since the last revision: improve error handling - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/f79c89da..7486ddb9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16920&range=06-07 Stats: 14 lines in 3 files changed: 1 ins; 6 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/16920.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16920/head:pull/16920 PR: https://git.openjdk.org/jdk/pull/16920
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v7]
On Wed, 20 Dec 2023 11:16:03 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: > > Spaces fix Hi, some requests and questions: - Please modify the JBS title, PR title, and JBS issue text to reflect that this adds an alternative shared object loading path for shared objects on AIX. Something like "Allow loading shared objects with .a extension on AIX". Please describe the new logic in the JBS issue text. - Does this really have to be handled in the OpenJDK? What does J9 on AIX do? Could this be done in a simpler way outside OpenJDK, e.g. by providing an *.so variant of the library in question? Where does this library come from? - What happens if we accidentally attempt to load a "real" static library, which is also named *.a? Would dlopen() then crash? What would happen? - What happens if the original path handed to os::dll_load is already a *.a file? Should the logic then be reversed? - We really need regression tests for this. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1864572287
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Mon, 18 Dec 2023 17:09:59 GMT, Serguei Spitsyn wrote:
>> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1
>> time frame.
>> It is fixing a deadlock issue between `VirtualThread` class critical
>> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`,
>> `getAndClearInterrupt()`, `threadState()`, `toString()`),
>> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms.
>> The deadlocking scenario is well described by Patricio in a bug report
>> comment.
>> In simple words, a virtual thread should not be suspended during
>> 'interruptLock' critical sections.
>>
>> The fix is to record that a virtual thread is in a critical section
>> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about
>> begin/end of critical section.
>> This bit is used in `HandshakeState::get_op_for_self()` to filter out any
>> `HandshakeOperation` if a target `JavaThread` is in a critical section.
>>
>> Some of new notifications with `notifyJvmtiSync()` method is on a
>> performance critical path. It is why this method has been intrincified.
>>
>> New test was developed by Patricio:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> The test is very nice as it reliably in 100% reproduces the deadlock without
>> the fix.
>> The test is never failing with this fix.
>>
>> Testing:
>> - tested with newly added test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 10 commits:
>
> - Merge
> - review: improve an assert message
> - review: moved a couple of comments out of try blocks
> - review: moved notifyJvmtiDisableSuspend(true) out of try-block
> - review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks
> - review: (1) rename notifyJvmti method; (2) add try-final statements to
> VirtualThread methods
> - Resolved merge conflict in VirtualThread.java
> - added @summary to new test SuspendWithInterruptLock.java
> - add new test SuspendWithInterruptLock.java
> - 8311218: fatal error: stuck in
> JvmtiVTMSTransitionDisabler::VTMS_transition_disable
src/hotspot/share/runtime/javaThread.hpp line 652:
> 650:
> 651: bool is_disable_suspend() const{ return
> _is_disable_suspend; }
> 652: void toggle_is_disable_suspend() { _is_disable_suspend =
> !_is_disable_suspend; };
Looking at this again then I don't think it can be a bit that is toggled on and
off will work. Consider the case where several threads attempt to poll the
state of a virtual Thread with Thread::getState at the same time. This can't
work without an atomic counter and further coordination. So I think further
work is required on this issue.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432770204
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v7]
On Wed, 20 Dec 2023 11:16:03 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:
>
> Spaces fix
Only some minor suggestions.
src/hotspot/os/aix/os_aix.cpp line 1168:
> 1166: int extension_length = 3;
> 1167: char* file_path = NEW_C_HEAP_ARRAY(char, buffer_length +
> extension_length + 1, mtInternal);
> 1168: strncpy(file_path,filename, buffer_length + 1);
Why not using
`char* file_path = os::strdup (filename);`
which would replace lines 1167+1168
and use the corresponding
`os::free (file_path);`
at the end
src/hotspot/os/aix/os_aix.cpp line 1174:
> 1172: result = dll_load_library(file_path, ebuf, ebuflen);
> 1173: // If the load fails,we try to reload by changing the extension to .a
> for .so files only.
> 1174: if(result == nullptr) {
Space between if and (
also next line
-
Changes requested by jkern (Author).
PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1790895382
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1432716207
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1432738451
Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
On Mon, 18 Dec 2023 22:48:18 GMT, Kim Barrett wrote: >> @eme64 Is it feasible to split this up to solve each of the problems you >> identify in stages? There is also overlap here with JDK-8319709 IIUC. Thanks. > >> @dholmes-ora These are the "parts": >> >> 1. initialize up to capacity vs length >> >> 2. update the test to verify this (complete refactoring) >> >> 3. remove cheap use of GrowableArray -> use GrowableArrayCHeap instead >> >> >> The first 2 items are inseparable, I cannot make substantial changes to many >> GrowableArray methods without there even being tests for them. And the tests >> would not pass before the changes for item 1, since the tests also verify >> what elements of the array are initialized. So adding the tests first would >> not be very feasible. >> >> The 3rd item could maybe be split, and be done before the rest. Though it >> would also require lots of changes to the test, which then I would have to >> completely refactor with items 1+2 anyway. >> >> And the items are related conceptually, that is why I would felt ok pushing >> them together. It is all about when (item 1) and what kinds of (item 3) >> constructors / destructors are called for the elements of the arrays, and >> verifying that thoroughly (item 2). >> >> Hence: feasible probably, but lots of work overhead. Do you think it is >> worth it? > > I too would prefer that it be split up. It's very easy to miss important > details in amongst all the mostly relatively > simple renamings. That is, I think 3 should be separate from the other > changes. @kimbarrett @dholmes-ora I just published this: https://github.com/openjdk/jdk/pull/17160 It removes the C-Heap capability from `GrowableArray`, and replaces usages with `GrowableArrayCHeap`. Bonus: we can now check that all element types of `GrowableArray` should be trivially destructible (that way it is always ok to abandon elements on the array, when the arena or ResourceMark go out of scope). - PR Comment: https://git.openjdk.org/jdk/pull/16918#issuecomment-1864429000
RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
[JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the `GrowableArrayCHeap`. This duplicates the current C-Heap allocation capability in `GrowableArray`. I now remove that from `GrowableArray` and move all usages to `GrowableArrayCHeap`. This has a few advantages: - Clear separation between arena (and resource area) allocating array and C-heap allocating array. - We can prevent assigning / copying between arrays of different allocation strategies already at compile time, and not only with asserts at runtime. - We should not have multiple implementations of the same thing (C-Heap backed array). - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now know that C-Heap backed arrays do not get copied unknowingly. **Bonus** We can now restrict `GrowableArray` element type `E` to be `std::is_trivially_destructible::value == true`. The idea is that arena / resource allocated arrays get abandoned, often without being even cleared. Hence, the elements in the array are never destructed. But if we only use elements that are trivially destructible, then it makes no difference if the destructors are ever called, or the elements simply abandoned. For `GrowableArrayCHeap`, we expect that the user eventually calls the destructor for the array, which in turn calls the destructors of the remaining elements. Hence, it is up to the user to ensure the cleanup. And so we can allow non-trivial destructors. **Testing** Tier1-3 + stress testing: pending - Commit messages: - fix comment about trivial elements - check for trivial destructors - improve comment - add an explicit to constructor - improve comments - remove cheap internals from GrowableArray and fix verification - remove constructors for GrowableArray with Cheap backing - 8322476 Changes: https://git.openjdk.org/jdk/pull/17160/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17160&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322476 Stats: 774 lines in 127 files changed: 108 ins; 204 del; 462 mod Patch: https://git.openjdk.org/jdk/pull/17160.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17160/head:pull/17160 PR: https://git.openjdk.org/jdk/pull/17160
Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow [v2]
On Tue, 19 Dec 2023 20:13:58 GMT, Chris Plummer wrote:
>> Jaikiran Pai has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - David's review comments - reduce if blocks and release the array outside
>> if block
>> - David's review comment - punctuation
>
> src/hotspot/os/posix/perfMemory_posix.cpp line 609:
>
>> 607: if (statbuf.st_size > 0 && statbuf.st_ctime > oldest_ctime) {
>> 608:
>> 609: if (statbuf.st_ctime > oldest_ctime) {
>
> This `if` expression repeats what we already know to be true from the
> previous `if` expression.
Hello Chris, you are right - this additional `if` isn't needed. It looks like
this is only there in `_posix` file and not in the `windows` version (which
also intentionally doesn't have the file size check). I've updated this PR to
fix this `_posix` one.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17104#discussion_r1432643800
Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow [v3]
On Wed, 20 Dec 2023 07:29:07 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to improve the code >> in `get_user_name_slow` function, which is used to identify the target JVM >> owner's user name? This addresses >> https://bugs.openjdk.org/browse/JDK-8321971. >> >> As noted in that JBS issue, in its current form, the nested loop ends up >> iterating over the directory contents of `hsperfdata_xxx` directory and then >> for each iteration it checks if the name of the entry matches the pid. This >> iteration shouldn't be needed and instead one could look for a file named >> `` within that directory. >> >> No new test has been added, given the nature of this change. Existing tier1, >> tier2, tier3 and svc_tools tests pass with this change on Linux, Windows and >> macosx. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - remove redundant if block > - merge latest from master branch > - David's review comments - reduce if blocks and release the array outside > if block > - David's review comment - punctuation > - 8321971: Improve the user name detection logic in perfMemory > get_user_name_slow Just a note - I have incorporated the review comments, except from Johan which I'm still investigating and will update this PR soon. - PR Comment: https://git.openjdk.org/jdk/pull/17104#issuecomment-1864379594
Re: [jdk22] RFR: 8321565: [REDO] Heap dump does not contain virtual Thread stack references
On Tue, 19 Dec 2023 22:48:28 GMT, Alex Menkov wrote: > Hi all, > > This pull request contains a backport of commit > [cf948548](https://github.com/openjdk/jdk/commit/cf948548c390c42ca63525d41a9d63ff31349c3a) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Alex Menkov on 13 Dec 2023 and > was reviewed by Serguei Spitsyn, Yi Yang and David Holmes. > > Thanks! This is a clean backport - approved. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk22/pull/21#pullrequestreview-1790704261
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v7]
> 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: Spaces fix - Changes: - all: https://git.openjdk.org/jdk/pull/16604/files - new: https://git.openjdk.org/jdk/pull/16604/files/9df8c2c8..ffcbf786 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16604&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16604&range=05-06 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16604/head:pull/16604 PR: https://git.openjdk.org/jdk/pull/16604
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 08:02:14 GMT, Alan Bateman wrote:
>> src/hotspot/share/prims/jvm.cpp line 4024:
>>
>>> 4022: #else
>>> 4023: fatal("Should only be called with JVMTI enabled");
>>> 4024: #endif
>>
>> You can't do this! The Java code knows nothing about JVM TI being
>> enabled/disabled and will call this function unconditionally.
>
>> You can't do this! The Java code knows nothing about JVM TI being
>> enabled/disabled and will call this function unconditionally.
>
> Indeed. I wonder if anyone is testing minimal builds to catch issues like
> this.
Good catch, David!
Filed a cleanup bug: https://bugs.openjdk.org/browse/JDK-8322538
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432548911
Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v16]
> Compiler Control (https://openjdk.org/jeps/165) provides method-context > dependent control of the JVM compilers (C1 and C2). The active directive > stack is built from the directive files passed with the > `-XX:CompilerDirectivesFile` diagnostic command-line option and the > Compiler.add_directives diagnostic command. It is also possible to clear all > directives or remove the top from the stack. > > A matching directive will be applied at method compilation time when such > compilation is started. If directives are added or changed, but compilation > does not start, then the state of compiled methods doesn't correspond to the > rules. This is not an error, and it happens in long running applications when > directives are added or removed after compilation of methods that could be > matched. For example, the user decides that C2 compilation needs to be > disabled for some method due to a compiler bug, issues such a directive but > this does not affect the application behavior. In such case, the target > application needs to be restarted, and such an operation can have high costs > and risks. Another goal is testing/debugging compilers. > > It would be convenient to optionally reconcile at least existing matching > nmethods to the current stack of compiler directives (so bypass inlined > methods). > > Natural way to eliminate the discrepancy between the result of compilation > and the broken rule is to discard the compilation result, i.e. > deoptimization. Prior to that we can try to re-compile the method letting > compile broker to perform it taking new directives stack into account. > Re-compilation helps to prevent hot methods from execution in the interpreter. > > A new flag `-r` has beed introduced for some directives related to compile > commands: `Compiler.add_directives`, `Compiler.remove_directives`, > `Compiler.clear_directives`. The default behavior has not changed (no flag). > If the new flag is present, the command scans already compiled methods and > puts methods that have any active non-default matching compiler directives to > re-compilation if possible, otherwise marks them for deoptimization. There is > currently no distinction which directives are found. In particular, this > means that if there are rules for inlining into some method, it will be > refreshed. On the other hand, if there are rules for a method and it was > inlined, top-level methods won't be refreshed, but this can be achieved by > having rules for them. > > In addition, a new diagnostic command `Compiler.replace_directives`, has been > added for ... Dmitry Chuyko has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 34 commits: - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - ... and 24 more: https://git.openjdk.org/jdk/compare/14dab319...e337e56b - Changes: https://git.openjdk.org/jdk/pull/14111/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14111&range=15 Stats: 372 lines in 15 files changed: 339 ins; 3 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/14111.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14111/head:pull/14111 PR: https://git.openjdk.org/jdk/pull/14111
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v2]
On Tue, 19 Dec 2023 16:47:33 GMT, Goetz Lindenmaier wrote: > try it! I got the instructions to replicate in my local repo later, so wasn't sure to proceed. Thanks for the suggestion. I think this makes it easier to keep in sync with the other change. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1864127477
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v2]
On Tue, 28 Nov 2023 11:27:33 GMT, Suchismith Roy wrote:
> > > i would have to repeat the line 1132 and 1139 in os_aix.cpp again , if
> > > the condition fails for .so files, because i have to reload it again and
> > > check if the .a exists. In the shared code i had repeat less number of
> > > lines i believe. Do you suggest moving lines 1132 to 1139 to another
> > > function then ?
> >
> >
> > @tstuefe Any suggestion on this ?
>
> ```
> --- a/src/hotspot/os/aix/os_aix.cpp
> +++ b/src/hotspot/os/aix/os_aix.cpp
> @@ -1108,7 +1108,7 @@ bool os::dll_address_to_library_name(address addr,
> char* buf,
>return true;
> }
>
> -void *os::dll_load(const char *filename, char *ebuf, int ebuflen) {
> +static void* dll_load_inner(const char *filename, char *ebuf, int ebuflen) {
>
>log_info(os)("attempting shared library load of %s", filename);
>
> @@ -1158,6 +1158,35 @@ void *os::dll_load(const char *filename, char *ebuf,
> int ebuflen) {
>return nullptr;
> }
>
> +void* os::dll_load(const char *filename, char *ebuf, int ebuflen) {
> +
> + void* result = nullptr;
> +
> + // First try using *.so suffix; failing that, retry with *.a suffix.
> + const size_t len = strlen(filename);
> + constexpr size_t safety = 3 + 1;
> + constexpr size_t bufsize = len + safety;
> + char* buf = NEW_C_HEAP_ARRAY(char, bufsize, mtInternal);
> + strcpy(buf, filename);
> + char* const dot = strrchr(buf, '.');
> +
> + assert(dot != nullptr, "Attempting to load a shared object without
> extension? %s", filename);
> + assert(strcmp(dot, ".a") == 0 || strcmp(dot, ".so") == 0,
> + "Attempting to load a shared object that is neither *.so nor *.a",
> filename);
> +
> + sprintf(dot, ".so");
> + result = dll_load_inner(buf, ebuf, ebuflen);
> +
> + if (result == nullptr) {
> +sprintf(dot, ".a");
> +result = dll_load_inner(buf, ebuf, ebuflen);
> + }
> +
> + FREE_C_HEAP_ARRAY(char, buf);
> +
> + return result;
> +}
> +
> ```
Hi Thomas
May I know what is the reason to use constexpr over regular datatypes ?
Also, I have used strcpy to avoid buffer overflow.(Though we have calculated
the exact length). Would that be fine ?
-
PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1864063261
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v6]
> 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 two additional commits since the last revision: - Restore lines - Remove trailing spaces. - Changes: - all: https://git.openjdk.org/jdk/pull/16604/files - new: https://git.openjdk.org/jdk/pull/16604/files/cd7e0e64..9df8c2c8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16604&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16604&range=04-05 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16604/head:pull/16604 PR: https://git.openjdk.org/jdk/pull/16604
Re: RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case
On Mon, 11 Dec 2023 14:38:08 GMT, Matthias Baesken wrote: >> A colleague contacted IBM about the different behavior of getaddrinfo on >> AIX (compared to Linux/macOS); maybe we have to adjust the result of the >> getaddrinfo call on AIX. > >> A colleague contacted IBM about the different behavior of getaddrinfo on AIX >> (compared to Linux/macOS); maybe we have to adjust the result of the >> getaddrinfo call on AIX. > > Haven't heard from them so far, hopefully we get an update soon about the > behavior of getaddrinfo on AIX . > @MBaesken Just a thought: parseAllowedAddr() needs to parse only numeric > addresses. getaddrinfo was used to handle both IPv4 and IPv6 by a single > call, but maybe it would be better to reimplement parseAllowedAddr to do 2 > inet_pton calls (for AF_INET and AF_INET6) Hi Alex, this seems to work (for AIX, and also for the other OpenJDK platforms) . - PR Comment: https://git.openjdk.org/jdk/pull/16561#issuecomment-1864029003
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 04:44:35 GMT, David Holmes wrote: > You can't do this! The Java code knows nothing about JVM TI being > enabled/disabled and will call this function unconditionally. Indeed. I wonder if anyone is testing minimal builds to catch issues like this. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432377494
