On Mon, 4 Nov 2024 14:52:06 GMT, Robert Toyonaga wrote:
>>>This include is not needed because there are no uses that require the
>>>definition of Thread.
>>
>> Right, seems like the forward declaration used to be provided by
>> `memory/allocation.hpp`. Let's get rid of the include and use a fo
On Mon, 4 Nov 2024 07:53:48 GMT, Johan Sjölen wrote:
>> src/hotspot/share/runtime/mutexLocker.hpp line 31:
>>
>>> 29: #include "runtime/flags/flagSetting.hpp"
>>> 30: #include "runtime/mutex.hpp"
>>> 31: #include "runtime/thread.hpp"
>>
>> This include is not needed because there are no uses th
On Fri, 1 Nov 2024 21:56:36 GMT, Markus Grönlund wrote:
>This include is not needed because there are no uses that require the
>definition of Thread.
Right, seems like the forward declaration used to be provided by
`memory/allocation.hpp`. Let's get rid of the include and use a forward
declar
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Thu, 31 Oct 2024 12:34:38 GMT, Thomas Stuefe wrote:
>>> I had to analyze this again, to understand why we need this locking, since
>>> my mind slips.
>>>
>>> I updated my findings in https://bugs.openjdk.org/browse/JDK-8325890 .
>>> Please see details there.
>>>
>>> But I don't think the c
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Thu, 31 Oct 2024 11:11:21 GMT, Johan Sjölen wrote:
> If this is so brittle and complex, then I wonder what you even get out of us
> doing the ThreadCritical trick. In other words, if we just removed it, would
> anyone notice and be able to discern what's occurred? Open question, I might
> d
On Thu, 31 Oct 2024 10:53:12 GMT, Thomas Stuefe wrote:
> I had to analyze this again, to understand why we need this locking, since my
> mind slips.
>
> I updated my findings in https://bugs.openjdk.org/browse/JDK-8325890 . Please
> see details there.
>
> But I don't think the current locking
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Wed, 30 Oct 2024 15:57:57 GMT, Robert Toyonaga wrote:
> So that means we'd need to have both ThreadCritical and NmtVirtualMemory_lock
> in that method (if we were to do the other replacements). One to protect the
> chunks and one to protect the malloc accounting. It might also be good to
>
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Wed, 30 Oct 2024 13:39:00 GMT, Robert Toyonaga wrote:
>I'm not certain, but looking at it again, it seems that the ThreadCritical
>uses in ChunkPool::deallocate_chunk and ChunkPool::prune() are only needed for
>NMT and are independent of the other ThreadCritical uses in that code.
At least
On Wed, 30 Oct 2024 12:32:17 GMT, Johan Sjölen wrote:
>>> Hi @tstuefe,
>>>
>>> > Ah thank you. I think I added that printing ages ago. I should not have
>>> > used tty. The immediate solution to get you going may be to just change
>>> > this from tty to fileStream fs(stderr);
>>>
>>> Thanks!
On Wed, 18 Sep 2024 05:53:05 GMT, Thomas Stuefe wrote:
>> Hi @roberttoyonaga
>>
>> (Pinging @afshin-zafari and @jdksjolen)
>>
>> First off, it is good that you ran into this since it highlights a possible
>> real deadlock. Both locks are quite coarse. I cannot think from the top of
>> my hea
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Fri, 25 Oct 2024 14:32:19 GMT, Thomas Stuefe wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/hotspot/share/utilities/vmError.cpp
>>
>> Co-authored-by: David Holmes
>> <62092539+dholmes-...@us
On Mon, 28 Oct 2024 16:03:32 GMT, Thomas Stuefe wrote:
>> Otherwise `make test TEST=hotspot_nmt` throws a lock rank error when running
>> the test `location_printing_mmap_1`. That test uses
>> `MemTracker::print_containing_region`.
>
> Thanks. But eew. Okay, ugly but not easy to fix.
> Can you
On Mon, 28 Oct 2024 15:50:00 GMT, Robert Toyonaga wrote:
>> Yes I think so. `MemTracker::print_containing_region` first acquires the
>> NMT lock then the `SharedDecoder_lock` like this:
>>
>> `MemTracker::print_containing_region` -->
>> `PrintRegionWalker::do_allocation_site` -->
>> `NativeCal
On Mon, 28 Oct 2024 16:09:44 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
> before VM init. There is also the possibility of addi
On Mon, 28 Oct 2024 15:45:31 GMT, Robert Toyonaga wrote:
>> src/hotspot/share/runtime/mutexLocker.cpp line 299:
>>
>>> 297: MUTEX_DEFN(ThreadsSMRDelete_lock , PaddedMonitor,
>>> service-2); // Holds ConcurrentHashTableResize_lock
>>> 298: MUTEX_DEFN(ThreadIdTableCreate_lock
On Fri, 25 Oct 2024 14:23:29 GMT, Thomas Stuefe wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/hotspot/share/utilities/vmError.cpp
>>
>> Co-authored-by: David Holmes
>> <62092539+dholmes-...@us
On Wed, 2 Oct 2024 13:28:13 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used e
On Sat, 12 Oct 2024 18:42:34 GMT, Afshin Zafari wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/hotspot/share/utilities/vmError.cpp
>>
>> Co-authored-by: David Holmes
>> <62092539+dholmes-...@us
On Sat, 12 Oct 2024 18:42:34 GMT, Afshin Zafari wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/hotspot/share/utilities/vmError.cpp
>>
>> Co-authored-by: David Holmes
>> <62092539+dholmes-...@us
On Wed, 2 Oct 2024 13:28:13 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used e
On Wed, 2 Oct 2024 13:28:13 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used e
On Wed, 2 Oct 2024 13:28:13 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used e
On Wed, 2 Oct 2024 02:34:29 GMT, David Holmes wrote:
> Okay I am happy with this now. Thanks.
Thank you for the review!
> If I heard correctly Thomas is on vacation this month so you may need to
> proceed with a different second review and follow up when he gets back if
> there are any proble
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
> before VM init. There is also the possibility of addi
On Thu, 26 Sep 2024 12:29:14 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
> before VM init. There is also the possibility of addi
On Sun, 22 Sep 2024 23:27:52 GMT, David Holmes wrote:
>> src/hotspot/share/runtime/mutexLocker.hpp line 261:
>>
>>> 259: // Same as MutexLocker but can be used during VM init.
>>> 260: // Performs no action if given a null mutex or with detached threads.
>>> 261: class NMTMutexLocker: public Con
On Sun, 22 Sep 2024 10:09:00 GMT, Thomas Stuefe wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Inherit from ConditionalMutexLocker. Use current_or_null_safe instead of
>> current_or_null.
>
> src/hotspot/share
On Fri, 20 Sep 2024 18:11:51 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Sun, 22 Sep 2024 10:15:29 GMT, Thomas Stuefe wrote:
>> src/hotspot/share/runtime/mutexLocker.hpp line 197:
>>
>>> 195: _mutex(mutex) {
>>> 196: bool no_safepoint_check = flag == Mutex::_no_safepoint_check_flag;
>>> 197: if (_mutex != nullptr && Thread::current_or_null() != nullptr
On Wed, 18 Sep 2024 00:26:24 GMT, David Holmes wrote:
>> Robert Toyonaga has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains five commits:
>>
>> - Merge master
>> - replace tty in pd_release_memory while holding NMT lock.
>> - Swit
On Fri, 20 Sep 2024 18:11:51 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Wed, 18 Sep 2024 05:53:05 GMT, Thomas Stuefe wrote:
> > Hi @tstuefe,
> > > Ah thank you. I think I added that printing ages ago. I should not have
> > > used tty. The immediate solution to get you going may be to just change
> > > this from tty to fileStream fs(stderr);
> >
> >
> > Thanks!
On Tue, 17 Sep 2024 23:16:47 GMT, David Holmes wrote:
>> Robert Toyonaga has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains five commits:
>>
>> - Merge master
>> - replace tty in pd_release_memory while holding NMT lock.
>> - Swit
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
> before VM init. There is also the possibility of addi
On Sat, 14 Sep 2024 06:56:01 GMT, Thomas Stuefe wrote:
>> Hi @tstuefe, it looks like calling
>> [`os::print_memory_mappings`](https://github.com/openjdk/jdk/blob/master/src/hotspot/os/windows/os_windows.cpp#L3785)
>> from the windows implementation of `os::pd_release_memory` is causing the
>>
On Tue, 17 Sep 2024 22:07:40 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
> before VM init. There is also the possibility of addi
On Sat, 14 Sep 2024 06:56:01 GMT, Thomas Stuefe wrote:
>> Hi @tstuefe, it looks like calling
>> [`os::print_memory_mappings`](https://github.com/openjdk/jdk/blob/master/src/hotspot/os/windows/os_windows.cpp#L3785)
>> from the windows implementation of `os::pd_release_memory` is causing the
>>
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
> before VM init. There is also the possibility of addi
On Sat, 14 Sep 2024 02:35:37 GMT, Robert Toyonaga wrote:
>>> > After switching to a Hotspot Mutex, it looks like the `windows-x64 / test
>>> > (hs/tier1 common) GHA` is failing because the test `release_bad_ranges`
>>> > in test_os.cpp is expecting an assertion and an error message to be
>>> >
On Fri, 13 Sep 2024 16:17:56 GMT, Thomas Stuefe wrote:
>>> After switching to a Hotspot Mutex, it looks like the `windows-x64 / test
>>> (hs/tier1 common) GHA` is failing because the test `release_bad_ranges` in
>>> test_os.cpp is expecting an assertion and an error message to be printed.
>>>
On Fri, 13 Sep 2024 16:12:59 GMT, Thomas Stuefe wrote:
> > After switching to a Hotspot Mutex, it looks like the `windows-x64 / test
> > (hs/tier1 common) GHA` is failing because the test `release_bad_ranges` in
> > test_os.cpp is expecting an assertion and an error message to be printed.
> >
On Fri, 13 Sep 2024 14:02:38 GMT, Robert Toyonaga wrote:
> After switching to a Hotspot Mutex, it looks like the `windows-x64 / test
> (hs/tier1 common) GHA` is failing because the test `release_bad_ranges` in
> test_os.cpp is expecting an assertion and an error message to be printed.
> Howeve
On Thu, 12 Sep 2024 14:37:22 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used
On Wed, 11 Sep 2024 06:10:11 GMT, Thomas Stuefe wrote:
>>> Thank you Robert.
>>>
>>> I think that the `MemoryFileTracker`'s locker should probably also be
>>> replaced with this semaphore, as in the future we have plans to have a
>>> globally shared `NativeCallStackStorage`.
>>
>> Hi @jdksjol
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
> before VM init. There is also the possibility of addi
On Sat, 7 Sep 2024 17:29:41 GMT, Robert Toyonaga wrote:
>>> Thank you Robert.
>>>
>>> I think that the `MemoryFileTracker`'s locker should probably also be
>>> replaced with this semaphore, as in the future we have plans to have a
>>> globally shared `NativeCallStackStorage`.
>>
>> +1
>
>> Th
On Thu, 5 Sep 2024 21:10:27 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used e
On Fri, 6 Sep 2024 20:18:36 GMT, Gerard Ziemski wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Comments. Hide assertion behind DEBUG. Replace MemoryFileTracker locker.
>> make reentrant.
>
> src/hotspot/share/
On Fri, 6 Sep 2024 10:35:39 GMT, Thomas Stuefe wrote:
>> We have a healthy amount of tests already. We need a wide use of
>> assert_locked and need to take a close look at error reporting. Locking, in
>> NMT, is rather simple.
>>
>> Error handling: My pragmatic "good enough for this rare scena
On Fri, 6 Sep 2024 10:36:15 GMT, Thomas Stuefe wrote:
> Thank you Robert.
>
> I think that the `MemoryFileTracker`'s locker should probably also be
> replaced with this semaphore, as in the future we have plans to have a
> globally shared `NativeCallStackStorage`.
Hi @jdksjolen. No problem! O
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
> before VM init. There is also the possibility of addi
On Wed, 4 Sep 2024 14:17:08 GMT, Robert Toyonaga wrote:
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
On Fri, 6 Sep 2024 08:32:01 GMT, Thomas Stuefe wrote:
>> src/hotspot/share/nmt/nmtCommon.hpp line 153:
>>
>>> 151: intx const current = os::current_thread_id();
>>> 152: intx const owner = Atomic::load(&_owner);
>>> 153: assert(current != owner, "Lock is not reentrant");
>>
>
On Thu, 5 Sep 2024 21:10:27 GMT, Robert Toyonaga wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used e
On Fri, 6 Sep 2024 08:51:31 GMT, Johan Sjölen wrote:
> Thank you Robert.
>
> I think that the `MemoryFileTracker`'s locker should probably also be
> replaced with this semaphore, as in the future we have plans to have a
> globally shared `NativeCallStackStorage`.
+1
-
PR Comment
On Wed, 4 Sep 2024 14:17:08 GMT, Robert Toyonaga wrote:
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
On Wed, 4 Sep 2024 14:17:08 GMT, Robert Toyonaga wrote:
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
On Fri, 6 Sep 2024 04:27:44 GMT, David Holmes wrote:
>> ### Summary
>> This PR just replaces `ThreadCritical` with a lock specific to NMT.
>> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
>> I've implemented the new lock with a semaphore so that it can be used earl
On Wed, 4 Sep 2024 14:17:08 GMT, Robert Toyonaga wrote:
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
On Wed, 4 Sep 2024 14:17:08 GMT, Robert Toyonaga wrote:
> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.
> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT.
> I've implemented the new lock with a semaphore so that it can be used early
### Summary
This PR just replaces `ThreadCritical` with a lock specific to NMT.
`ThreadCritical` is a big lock and is unnecessary for the purposes of NMT. I've
implemented the new lock with a semaphore so that it can be used early before
VM init. There is also the possibility of adding asserti
73 matches
Mail list logo
