Integrated: 8337563: NMT: rename MEMFLAGS to MemTag
On Thu, 5 Sep 2024 16:10:05 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related function/template > parameter names and local variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) This pull request has now been integrated. Changeset: eabfc6e4 Author:Gerard Ziemski URL: https://git.openjdk.org/jdk/commit/eabfc6e4d901c53b93a78da740ca376607d9576d Stats: 1533 lines in 127 files changed: 144 ins; 138 del; 1251 mod 8337563: NMT: rename MEMFLAGS to MemTag Reviewed-by: dholmes, coleenp, jsjolen - PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v11]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related function/template > parameter names and local variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with two additional commits since the last revision: - copyrights - Afshin's feedback, tests - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/1765579b..7db989f3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=09-10 Stats: 9 lines in 7 files changed: 1 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v10]
On Thu, 12 Sep 2024 15:40:35 GMT, Gerard Ziemski wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemTag` is a more suitable name. >> >> This fix also includes a cleanup of all the related function/template >> parameter names and local variable names. >> >> Testing is pending... >> >> Note: there is more history in old closed PRs >> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) >> and >> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) > > Gerard Ziemski has updated the pull request incrementally with two additional > commits since the last revision: > > - Afshin's feedback, record_virtual_memory_type -> record_virtual_memory_tag > - Afshin's feedback Johan's incremental feedback: https://openjdk.github.io/cr/?repo=jdk&pr=20872&range=06-07 Afshin's incremental feedback: https://openjdk.github.io/cr/?repo=jdk&pr=20872&range=07-08 https://openjdk.github.io/cr/?repo=jdk&pr=20872&range=08-09 - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2346643064
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v9]
On Thu, 12 Sep 2024 15:32:57 GMT, Gerard Ziemski wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemTag` is a more suitable name. >> >> This fix also includes a cleanup of all the related function/template >> parameter names and local variable names. >> >> Testing is pending... >> >> Note: there is more history in old closed PRs >> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) >> and >> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) > > Gerard Ziemski has updated the pull request incrementally with one additional > commit since the last revision: > > Afshin's feedback I have incorporated Afshin feedback, but can revert the changes and punt them to a followup if there is a pushback against doing it right now. I think that we are now in pretty good shape. I could use final approvals, with anything that's not critical to be done in followup(s) later. - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2346637453
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v9]
On Thu, 12 Sep 2024 15:32:57 GMT, Gerard Ziemski wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemTag` is a more suitable name. >> >> This fix also includes a cleanup of all the related function/template >> parameter names and local variable names. >> >> Testing is pending... >> >> Note: there is more history in old closed PRs >> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) >> and >> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) > > Gerard Ziemski has updated the pull request incrementally with one additional > commit since the last revision: > > Afshin's feedback Thank you Afshin again for taking detailed look. - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2346633208
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v7]
On Thu, 12 Sep 2024 13:31:28 GMT, Afshin Zafari wrote: > ## Again, if we want to replace mem type with mem tag. The regexp search for > mem.*type gives this: > 47 results - 19 files Done. > The `MemTracker::record_virtual_memory_type` can be changed to > `MemTracker::record_virtual_memory_tag` Done. - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2346629838 PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2346630233
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v10]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related function/template > parameter names and local variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with two additional commits since the last revision: - Afshin's feedback, record_virtual_memory_type -> record_virtual_memory_tag - Afshin's feedback - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/8e576cf1..1765579b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=08-09 Stats: 42 lines in 24 files changed: 0 ins; 0 del; 42 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v9]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related function/template > parameter names and local variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision: Afshin's feedback - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/4c7f181d..8e576cf1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=07-08 Stats: 26 lines in 14 files changed: 0 ins; 0 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v7]
On Wed, 11 Sep 2024 14:03:17 GMT, Gerard Ziemski wrote:
>> Gerard Ziemski has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Coleen's feedback
>
> Are we sure we want `mt` for non-type parameter name in templates? We have
> these existing patterns already in our code:
>
>
> src/hotspot/share/utilities/growableArray.hpp:803:template MemTag MT>
> src/hotspot/share/utilities/stack.hpp:54:template class
> StackIterator;
> src/hotspot/share/utilities/concurrentHashTable.inline.hpp:78:template
>
> src/hotspot/share/utilities/chunkedList.hpp:31:template
> class ChunkedList : public CHeapObj
> src/hotspot/share/gc/g1/g1BatchedTask.hpp:32:template
> src/hotspot/share/gc/shared/taskqueue.hpp:119:template MemTag MT>
> src/hotspot/share/gc/shared/taskqueue.hpp:327:template unsigned int N = TASKQUEUE_SIZE>
> src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.hpp:40:template MemTag MT, unsigned int N = TASKQUEUE_SIZE>
> src/hotspot/share/nmt/arrayWithFreeList.hpp:34:template
>
>
> With mt they would look like:
>
>
> src/hotspot/share/utilities/growableArray.hpp:803:template MemTag mt>
> src/hotspot/share/utilities/stack.hpp:54:template class
> StackIterator;
> src/hotspot/share/utilities/concurrentHashTable.inline.hpp:78:template
>
> src/hotspot/share/utilities/chunkedList.hpp:31:template
> class ChunkedList : public CHeapObj
> src/hotspot/share/gc/g1/g1BatchedTask.hpp:32:template
> src/hotspot/share/gc/shared/taskqueue.hpp:119:template MemTag mt>
> src/hotspot/share/gc/shared/taskqueue.hpp:327:template unsigned int N = TASKQUEUE_SIZE>
> src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.hpp:40:template MemTag mt, unsigned int N = TASKQUEUE_SIZE>
> src/hotspot/share/nmt/arrayWithFreeList.hpp:34:template
>
>
> So `MT` or `mt` for non-type parameter name in templates, or should I punt on
> this particular change and leave it for a followup?
> Thank you @gerard-ziemski, for this huge change. After this change, the code
> looks much more nicer and consistent.
>
> If we are insisting on replacing `flag` with `tag`, I could find these missed
> ones by regexp search for `mem.*flag`:
>
> 7 results - 5 files
>
> Source root • src/hotspot/share/nmt/memMapPrinter.cpp: `83: // A Cache that
> correlates range with MEMFLAG, optimized to be iterated quickly`
>
> Source root • src/hotspot/share/nmt/memTracker.hpp: `208: // memory flags of
> the original region.`
>
> Source root • src/hotspot/share/nmt/vmatree.hpp:
>
> `97: assert(!(type == StateType::Released) || data.mem_tag == mtNone,
> "Released type must have flag mtNone");`
>
> `108: return static_cast(type_flag[1]);`
>
> Source root • test/hotspot/gtest/nmt/test_nmt_reserved_region.cpp: `50:
> ASSERT_EQ(region2.mem_tag(), mtThreadStack); // Should be correct flag`
>
> Source root • test/hotspot/gtest/nmt/test_vmatree.cpp:
>
> `435: const MemTag candidate_flags[candidates_len_flags] = {`
>
> `459: const MemTag mem_tag = candidate_flags[os::random() %
> candidates_len_flags];`
Thank you Ashfin for laking such a detailed look and providing the feedback.
I fixed the issues, except:
`108: return static_cast(type_flag[1]);`
This actually has more than just MemTag, it also has StateType, so I left it as
is.
-
PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2346602184
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v8]
On Thu, 12 Sep 2024 11:54:49 GMT, David Holmes wrote: >> src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.inline.hpp line 2: >> >>> 1: /* >>> 2: * Copyright (c) 2016, 2019, Red Hat, Inc. All rights reserved. >> >> I don't think we're meant to update other companies' copyrights? > > That is correct - unless requested by someone representing that copyright > owner. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1757091561
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v8]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related function/template > parameter names and local variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision: Johan's feedback - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/f1faba35..4c7f181d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=06-07 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v7]
On Tue, 10 Sep 2024 20:53:46 GMT, Gerard Ziemski wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemTag` is a more suitable name. >> >> This fix also includes a cleanup of all the related function/template >> parameter names and local variable names. >> >> Testing is pending... >> >> Note: there is more history in old closed PRs >> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) >> and >> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) > > Gerard Ziemski has updated the pull request incrementally with one additional > commit since the last revision: > > Coleen's feedback Are we sure we want `mt` for non-type parameter name in templates? We have these existing patterns already in our code: src/hotspot/share/utilities/growableArray.hpp:803:template src/hotspot/share/utilities/stack.hpp:54:template class StackIterator; src/hotspot/share/utilities/concurrentHashTable.inline.hpp:78:template src/hotspot/share/utilities/chunkedList.hpp:31:template class ChunkedList : public CHeapObj src/hotspot/share/gc/g1/g1BatchedTask.hpp:32:template src/hotspot/share/gc/shared/taskqueue.hpp:119:template src/hotspot/share/gc/shared/taskqueue.hpp:327:template src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.hpp:40:template src/hotspot/share/nmt/arrayWithFreeList.hpp:34:template With mt they would look like: src/hotspot/share/utilities/growableArray.hpp:803:template src/hotspot/share/utilities/stack.hpp:54:template class StackIterator; src/hotspot/share/utilities/concurrentHashTable.inline.hpp:78:template src/hotspot/share/utilities/chunkedList.hpp:31:template class ChunkedList : public CHeapObj src/hotspot/share/gc/g1/g1BatchedTask.hpp:32:template src/hotspot/share/gc/shared/taskqueue.hpp:119:template src/hotspot/share/gc/shared/taskqueue.hpp:327:template src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.hpp:40:template src/hotspot/share/nmt/arrayWithFreeList.hpp:34:template So `MT` or `mt` for non-type parameter name in templates, or should I punt on this particular change and leave it for a followup? - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2343769766
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v7]
On Tue, 10 Sep 2024 20:53:46 GMT, Gerard Ziemski wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemTag` is a more suitable name. >> >> This fix also includes a cleanup of all the related function/template >> parameter names and local variable names. >> >> Testing is pending... >> >> Note: there is more history in old closed PRs >> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) >> and >> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) > > Gerard Ziemski has updated the pull request incrementally with one additional > commit since the last revision: > > Coleen's feedback Thank you for the feedback Coleen! - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2341999055
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v7]
On Tue, 10 Sep 2024 20:53:46 GMT, Gerard Ziemski wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemTag` is a more suitable name. >> >> This fix also includes a cleanup of all the related parameter names and >> local variable names. >> >> Testing is pending... >> >> Note: there is more history in old closed PRs >> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) >> and >> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) > > Gerard Ziemski has updated the pull request incrementally with one additional > commit since the last revision: > > Coleen's feedback I went with `MT` throughout the code for all MemTag template parameter names. It will be a nice mental shortcut once we get used to the change... I did not want to rock the boat too much and decided not to use `mem_tag` for template parameter names, it just did not look good next to the other names, with all capital letters. - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2341992426
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v5]
On Tue, 10 Sep 2024 20:28:01 GMT, Coleen Phillimore wrote: >> Gerard Ziemski has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fix test > > src/hotspot/share/nmt/memTracker.hpp line 265: > >> 263: >> 264: // MallocLimt: Given an allocation size s, check if mallocing this >> much >> 265: // under category f would hit either the global limit or the limit >> for mem_tag. > > I don't know what "category f" is. Maybe reword as > > // MallocLimit: Given an allocation size s, check if allocating this much > memory would hit the global limit or the > // limit tagged with mem_tag. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1752704620
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v7]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision: Coleen's feedback - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/c779754c..f1faba35 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=05-06 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v6]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision: rename MemoryTag template parameter names - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/7a4f6e01..c779754c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=04-05 Stats: 400 lines in 19 files changed: 2 ins; 0 del; 398 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag
On Tue, 10 Sep 2024 05:57:08 GMT, David Holmes wrote: > > The template parameter rename I was planning on doing in a followup issue, > > however, if you really want, I can make the fix here too. > > Personally I'd be okay with doing it here as one final commit that can be > viewed in isolation. Sure, I can do it. Is everyone OK with MT as the template parameter name? Another obvious choice is MEM_TAG Side note: why are template parameter names all capitals? To help distinguish them from "regular" parameters? Do we still want that naming scheme, or can we switch here to using mem_tag? - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2340957192
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v5]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision: fix test - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/bf062da9..7a4f6e01 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=03-04 Stats: 24 lines in 1 file changed: 0 ins; 0 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v4]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision: revert type->state change - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/998537bd..bf062da9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=02-03 Stats: 21 lines in 3 files changed: 0 ins; 0 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v3]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision: fix linux build issue - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/9ae36d57..998537bd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag [v2]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision: fix incorrect renames, missing type -> mem_tag - Changes: - all: https://git.openjdk.org/jdk/pull/20872/files - new: https://git.openjdk.org/jdk/pull/20872/files/983bb6e2..9ae36d57 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=00-01 Stats: 28 lines in 9 files changed: 0 ins; 1 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag
On Thu, 5 Sep 2024 16:10:05 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) Thank you David, Kim for your feedback, as the very first reviewers your job was the hardest. I implemented all your feedback. The template parameter rename I was planning on doing in a followup issue, however, if you really want, I can make the fix here too. It will increase the size of the changes, but I already accommodated Stefan request to include parameters and local variables, so we can go this one last step further if you like. - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2338647692
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag
On Sat, 7 Sep 2024 05:21:50 GMT, Kim Barrett wrote:
>> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`.
>>
>> `MEMFLAGS` implies that we can use more than one at the same time, but those
>> are exclusive values, so `MemTag` is a more suitable name.
>>
>> This fix also includes a cleanup of all the related parameter names and
>> local variable names.
>>
>> Testing is pending...
>>
>> Note: there is more history in old closed PRs
>> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497)
>> and
>> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472)
>
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 63:
>
>> 61: static void* allocate_node(void* context, size_t size, Value const&
>> value) {
>> 62: ObjectMonitorTable::inc_items_count();
>> 63: return AllocateHeap(size, MemTag::mtObjectMonitor);
>
> pre-existing: Why the scope here and below?
Good question, I will clean this up.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750614387
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag
On Mon, 9 Sep 2024 00:04:58 GMT, David Holmes wrote: >> src/hotspot/share/gc/shared/taskqueue.hpp line 119: >> >>> 117: // TaskQueueSuper collects functionality common to all >>> GenericTaskQueue instances. >>> 118: >>> 119: template >> >> MemTag parameter name should probably be changed here and elsewhere in >> taskqueue code. >> Suggest `mem_tag`. > > I was going to suggest just MT which is more in keeping with the short/terse > names given to type parameters. Will be address in a followup issue. - PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750615114
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag
On Mon, 9 Sep 2024 13:36:26 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/utilities/chunkedList.hpp line 31:
>>
>>> 29: #include "utilities/debug.hpp"
>>> 30:
>>> 31: template class ChunkedList : public CHeapObj {
>>
>> Parameter name should be updated. Suggest `mem_tag`.
>
> How about MT here or just M? I would make this a further change though.
We can change this later in a followup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750604170
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag
On Sat, 7 Sep 2024 05:24:29 GMT, Kim Barrett wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemTag` is a more suitable name. >> >> This fix also includes a cleanup of all the related parameter names and >> local variable names. >> >> Testing is pending... >> >> Note: there is more history in old closed PRs >> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) >> and >> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) > > src/hotspot/share/runtime/os.hpp line 918: > >> 916: static ssize_t recv(int fd, char* buf, size_t nBytes, uint type); >> 917: static ssize_t send(int fd, char* buf, size_t nBytes, uint type); >> 918: static ssize_t raw_send(int fd, char* buf, size_t nBytes, uint type); > > This set of changes is wrong. These aren't MEMFLAGS flags. > (I hope there aren't any more like this. This sort of thing would be easy to > miss in a change this > large. If I were making this change I'd have broken it up into several > smaller pieces.) Looks like a find/replace issue. Reverted the change. > src/hotspot/share/utilities/concurrentHashTable.hpp line 43: > >> 41: class Mutex; >> 42: >> 43: template > > Parameter name should be updated throughout ConcurrentHashTable. Suggest > mem_tag. We can change this later in a followup. > src/hotspot/share/utilities/growableArray.hpp line 803: > >> 801: >> 802: // Leaner GrowableArray for CHeap backed data arrays, with compile-time >> decided MemTag. >> 803: template > > Another parameter needing update, but shouldn't (can't?) be called `mem_tag` > because of the > function parameter name for allocate(). We can change this later in a followup. > src/hotspot/share/utilities/linkedlist.hpp line 368: > >> 366: template > 367: AnyObj::allocation_type T = AnyObj::C_HEAP, >> 368: MemTag F = mtNMT, AllocFailType alloc_failmode = >> AllocFailStrategy::RETURN_NULL> > > Another parameter name needing update. We can change this later in a followup. > src/hotspot/share/utilities/objectBitSet.hpp line 42: > >> 40: * during the lifetime of the ObjectBitSet. The underlying memory is >> allocated from C-Heap. >> 41: */ >> 42: template > > More parameter names needing update. I followed the local pattern, but we can change this later in a followup. > src/hotspot/share/utilities/resizeableResourceHash.hpp line 33: > >> 31: typename K, typename V, >> 32: AnyObj::allocation_type ALLOC_TYPE, >> 33: MemTag MEM_TYPE> > > I think s/MEM_TYPE/mem_type/, but other non-type template parameters here are > also > all-uppercase, so I guess better to leave it for now and look at it later. Yes, I followed the local pattern. - PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750606609 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750603957 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750603545 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750603358 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750602937 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750601971
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag
On Mon, 9 Sep 2024 00:13:25 GMT, David Holmes wrote:
>> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`.
>>
>> `MEMFLAGS` implies that we can use more than one at the same time, but those
>> are exclusive values, so `MemTag` is a more suitable name.
>>
>> This fix also includes a cleanup of all the related parameter names and
>> local variable names.
>>
>> Testing is pending...
>>
>> Note: there is more history in old closed PRs
>> [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497)
>> and
>> [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472)
>
> src/hotspot/share/nmt/memMapPrinter.cpp line 70:
>
>> 68: //end
>> 69:
>> 70: static const char* get_shortname_for_nmt_flag(MemTag mem_tag) {
>
> Shouldn't this be renamed to `get_shortname_for_nmt_tag`?
I went with `get_shortname_for_mem_tag()` I think that is more consistent? Is
that OK?
> src/hotspot/share/nmt/memReporter.cpp line 852:
>
>> 850: } else if (early_site->mem_tag() != current_site->mem_tag()) {
>> 851: // This site was originally allocated with one type, then
>> released,
>> 852: // then re-allocated at the same site (as far as we can tell)
>> with a different type.
>
> s/type/tag/
Fixed.
> src/hotspot/share/nmt/memTracker.hpp line 83:
>
>> 81: if (enabled()) {
>> 82: return MallocTracker::record_malloc(mem_base, size, mem_tag,
>> stack);
>> 83: return MallocTracker::record_malloc(mem_base, size, mem_tag,
>> stack);
>
> Did this even compile? !
> Suggestion:
>
> return MallocTracker::record_malloc(mem_base, size, mem_tag, stack);
Fixed. None of the compilers had a problem with this weirdly enough.
> src/hotspot/share/nmt/memoryFileTracker.cpp line 51:
>
>> 49: for (int i = 0; i < mt_number_of_tags; i++) {
>> 50: VirtualMemory* summary =
>> file->_summary.by_type(NMTUtil::index_to_tag(i));
>> 51: summary->reserve_memory(diff.type[i].commit);
>
> Why is this `type` not `tag`?
Fixed.
> src/hotspot/share/nmt/memoryFileTracker.cpp line 109:
>
>> 107: tty->print_cr("Expected start out to have same type as end in, but
>> was: %s, %s",
>> 108:
>> VMATree::statetype_to_string(broken_start->val().out.state()),
>> 109:
>> VMATree::statetype_to_string(broken_end->val().in.state()));
>
> Not seeing what this rename has to do with current changes. ???
Fixed.
> src/hotspot/share/nmt/virtualMemoryTracker.cpp line 400:
>
>> 398:
>> 399: // Print some more details. Don't use UL here to avoid
>> circularities.
>> 400: tty->print_cr("Error: existing region: [" INTPTR_FORMAT "-"
>> INTPTR_FORMAT "), type %u.\n"
>
> Again why `type` instead of `tag`?
Fixed.
> src/hotspot/share/nmt/virtualMemoryTracker.cpp line 560:
>
>> 558: // Given an existing memory mapping registered with NMT, split the
>> mapping in
>> 559: // two. The newly created two mappings will be registered under the
>> call
>> 560: // stack and the memory types of the original section.
>
> types -> tags
Fixed.
> src/hotspot/share/nmt/vmatree.cpp line 86:
>
>> 84: // If the state is not matching then we have different
>> operations, such as:
>> 85: // reserve [x1, A); ... commit [A, x2); or
>> 86: // reserve [x1, A), type1; ... reserve [A, x2), type2; or
>
> Why type not tag?
I will fix it.
> src/hotspot/share/nmt/vmatree.hpp line 91:
>
>> 89: private:
>> 90: // Store the state and mem_tag as two bytes
>> 91: uint8_t info[2];
>
> I'm unclear about terminology here: type -> state ?
Those come from my initial fix, when I renamed `MEMFLAGS` -> `MemType`.
In that work the type_flag was clashing with mem_type parameter, so I renamed
it from `type` to `state`
I tried to re-use that original patch, but as you just found, some of those
changes do not apply.
I will undo this change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750598453
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750596573
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750595789
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750592818
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750589625
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750588916
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750587807
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750586650
PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750583730
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemTag
On Thu, 5 Sep 2024 16:10:05 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) @dholmes-ora @tstuefe @coleenp @stefank @kimbarrett @jdksjolen @afshin-zafari MEMFLAGS rename task has been moved here. - PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2334800974
RFR: 8337563: NMT: rename MEMFLAGS to MemTag
Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. `MEMFLAGS` implies that we can use more than one at the same time, but those are exclusive values, so `MemTag` is a more suitable name. This fix also includes a cleanup of all the related parameter names and local variable names. Testing is pending... Note: there is more history in old closed PRs [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) and [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) - Commit messages: - missed renames on Windows - fix test failure - fix incorrect rename - mssing renames - missed _mem_type --> _mem_tags - missed MemFlagBitmap --> MemTagBitmap - missed tag --> mem_tag - missed type --> tag - missed type --> tag - missed flag_is_valid --> tag_is_valid - ... and 1 more: https://git.openjdk.org/jdk/compare/4ffcf894...983bb6e2 Changes: https://git.openjdk.org/jdk/pull/20872/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20872&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337563 Stats: 1259 lines in 108 files changed: 142 ins; 138 del; 979 mod Patch: https://git.openjdk.org/jdk/pull/20872.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20872/head:pull/20872 PR: https://git.openjdk.org/jdk/pull/20872
Re: RFR: 8304824: NMT should not use ThreadCritical
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
> before VM init. There is also the possibility of adding assertions in places
> we expect NMT to have synchronization. I haven't added assertions yet in many
> places because some code paths such as the (NMT tests) don't lock yet. I
> think it makes sense to close any gaps in locking in another PR in which I
> can also add more assertions.
>
> Testing:
> - hotspot_nmt
> - gtest:VirtualSpace
> - tier1
Changes requested by gziemski (Committer).
src/hotspot/share/nmt/nmtCommon.hpp line 168:
> 166: intx const current = os::current_thread_id();
> 167: intx const owner = Atomic::load(&_owner);
> 168: assert(current == owner, "NMT lock should be acquired in this
> section.");
How about we try to re-use `assert_locked()` in constructor and destructor,
something like:
NmtGuard() {
assert(!assert_locked(), "Lock is not reentrant");
_nmt_semaphore.wait();
Atomic::store(&_owner, current);
}
~NmtGuard() {
assert(assert_locked(), "NMT lock should be acquired in this section.");
Atomic::store(&_owner, (intx) -1);
_nmt_semaphore.signal();
}
static bool assert_locked(){
intx const current = os::current_thread_id();
intx const owner = Atomic::load(&_owner);
return (current == owner);
}
-
PR Review: https://git.openjdk.org/jdk/pull/20852#pullrequestreview-2287095347
PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1747670483
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag [v2]
On Wed, 4 Sep 2024 21:17:28 GMT, Gerard Ziemski wrote:
>> Please review this cleanup, where we rename `MEMFLAGS` to `MemType`.
>>
>> `MEMFLAGS` implies that we can use more than one at the same time, but those
>> are exclusive values, so `MemType` is much more suitable name.
>>
>> There is a bunch of other related cleanup that we can do, but I will leave
>> for follow up issues such as [NMT: rename NMTUtil::flag to
>> NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836)
>
> Gerard Ziemski has updated the pull request incrementally with 308 additional
> commits since the last revision:
>
> - undo MEMFLAGS to MemType
> - 8339233: Test javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java#id
> failed: Button renderings are different after window resize
>
>Reviewed-by: honkar
> - 8338924: C1: assert(0 <= i && i < _len) failed: illegal index 5 for length
> 5
>
>Co-authored-by: Dean Long
>Reviewed-by: kvn, thartmann
> - 8339492: StackMapDecoder::writeFrames makes lots of allocations
>
>Reviewed-by: liach, redestad, jwaters, asotona
> - 8332901: Select{Current,New}ItemTest.java for Choice don't open popup on
> macOS
>
>Move SelectCurrentItemTest.java to java/awt/Choice/SelectItem/.
>Move SelectNewItemTest.java to java/awt/Choice/SelectItem/.
>Use latches to control test flow instead of delays.
>Encapsulate the common logic in SelectCurrentItemTest.
>Provide overridable checkXXX() methods to modify conditions.
>Provide an overridable method which defines where to click
>in the choice popup to select an item.
>
>Reviewed-by: honkar, prr, dnguyen
> - 8339148: Make os::Linux::active_processor_count() public
>
>Reviewed-by: dholmes, jwaters
> - 8339112: Move JVM Klass flags out of AccessFlags
>
>Reviewed-by: matsaave, cjplummer, dlong, thartmann, yzheng
> - 8336860: x86: Change integer src operand for CMoveL of 0 and 1 to long
>
>Reviewed-by: epeter, chagedorn, shade, qamai, jbhateja
> - 8325679: Optimize ArrayList subList sort
>
>Reviewed-by: liach
> - 8339131: Remove rarely-used accessor methods from Opcode
>
>Reviewed-by: asotona
> - ... and 298 more: https://git.openjdk.org/jdk/compare/9665d7f7...6d6d70e9
I'm not as excited about it as I was when started working on it.
Will get https://github.com/openjdk/jdk/pull/20872 ready, then will give it
final thought.
-
PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2334578610
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag [v2]
On Wed, 4 Sep 2024 21:17:28 GMT, Gerard Ziemski wrote:
>> Please review this cleanup, where we rename `MEMFLAGS` to `MemType`.
>>
>> `MEMFLAGS` implies that we can use more than one at the same time, but those
>> are exclusive values, so `MemType` is much more suitable name.
>>
>> There is a bunch of other related cleanup that we can do, but I will leave
>> for follow up issues such as [NMT: rename NMTUtil::flag to
>> NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836)
>
> Gerard Ziemski has updated the pull request incrementally with 308 additional
> commits since the last revision:
>
> - undo MEMFLAGS to MemType
> - 8339233: Test javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java#id
> failed: Button renderings are different after window resize
>
>Reviewed-by: honkar
> - 8338924: C1: assert(0 <= i && i < _len) failed: illegal index 5 for length
> 5
>
>Co-authored-by: Dean Long
>Reviewed-by: kvn, thartmann
> - 8339492: StackMapDecoder::writeFrames makes lots of allocations
>
>Reviewed-by: liach, redestad, jwaters, asotona
> - 8332901: Select{Current,New}ItemTest.java for Choice don't open popup on
> macOS
>
>Move SelectCurrentItemTest.java to java/awt/Choice/SelectItem/.
>Move SelectNewItemTest.java to java/awt/Choice/SelectItem/.
>Use latches to control test flow instead of delays.
>Encapsulate the common logic in SelectCurrentItemTest.
>Provide overridable checkXXX() methods to modify conditions.
>Provide an overridable method which defines where to click
>in the choice popup to select an item.
>
>Reviewed-by: honkar, prr, dnguyen
> - 8339148: Make os::Linux::active_processor_count() public
>
>Reviewed-by: dholmes, jwaters
> - 8339112: Move JVM Klass flags out of AccessFlags
>
>Reviewed-by: matsaave, cjplummer, dlong, thartmann, yzheng
> - 8336860: x86: Change integer src operand for CMoveL of 0 and 1 to long
>
>Reviewed-by: epeter, chagedorn, shade, qamai, jbhateja
> - 8325679: Optimize ArrayList subList sort
>
>Reviewed-by: liach
> - 8339131: Remove rarely-used accessor methods from Opcode
>
>Reviewed-by: asotona
> - ... and 298 more: https://git.openjdk.org/jdk/compare/9665d7f7...6d6d70e9
The PR has been moved to https://github.com/openjdk/jdk/pull/20872
-
PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2332134475
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag [v2]
On Wed, 4 Sep 2024 21:17:28 GMT, Gerard Ziemski wrote:
>> Please review this cleanup, where we rename `MEMFLAGS` to `MemType`.
>>
>> `MEMFLAGS` implies that we can use more than one at the same time, but those
>> are exclusive values, so `MemType` is much more suitable name.
>>
>> There is a bunch of other related cleanup that we can do, but I will leave
>> for follow up issues such as [NMT: rename NMTUtil::flag to
>> NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836)
>
> Gerard Ziemski has updated the pull request incrementally with 308 additional
> commits since the last revision:
>
> - undo MEMFLAGS to MemType
> - 8339233: Test javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java#id
> failed: Button renderings are different after window resize
>
>Reviewed-by: honkar
> - 8338924: C1: assert(0 <= i && i < _len) failed: illegal index 5 for length
> 5
>
>Co-authored-by: Dean Long
>Reviewed-by: kvn, thartmann
> - 8339492: StackMapDecoder::writeFrames makes lots of allocations
>
>Reviewed-by: liach, redestad, jwaters, asotona
> - 8332901: Select{Current,New}ItemTest.java for Choice don't open popup on
> macOS
>
>Move SelectCurrentItemTest.java to java/awt/Choice/SelectItem/.
>Move SelectNewItemTest.java to java/awt/Choice/SelectItem/.
>Use latches to control test flow instead of delays.
>Encapsulate the common logic in SelectCurrentItemTest.
>Provide overridable checkXXX() methods to modify conditions.
>Provide an overridable method which defines where to click
>in the choice popup to select an item.
>
>Reviewed-by: honkar, prr, dnguyen
> - 8339148: Make os::Linux::active_processor_count() public
>
>Reviewed-by: dholmes, jwaters
> - 8339112: Move JVM Klass flags out of AccessFlags
>
>Reviewed-by: matsaave, cjplummer, dlong, thartmann, yzheng
> - 8336860: x86: Change integer src operand for CMoveL of 0 and 1 to long
>
>Reviewed-by: epeter, chagedorn, shade, qamai, jbhateja
> - 8325679: Optimize ArrayList subList sort
>
>Reviewed-by: liach
> - 8339131: Remove rarely-used accessor methods from Opcode
>
>Reviewed-by: asotona
> - ... and 298 more: https://git.openjdk.org/jdk/compare/9665d7f7...6d6d70e9
Closing this PR and will move to a new one shortly.
-
PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2332003251
Withdrawn: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/20497
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) I tried undoing my MEMFLAGS to MemType change in preparations for MEMFLAGS to MemTag rename (which I have working), but that caused massive rebase, so I will close this PR and move it to a clean branch if no-one objects? - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2330132485
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag [v2]
> Please review this cleanup, where we rename `MEMFLAGS` to `MemType`.
>
> `MEMFLAGS` implies that we can use more than one at the same time, but those
> are exclusive values, so `MemType` is much more suitable name.
>
> There is a bunch of other related cleanup that we can do, but I will leave
> for follow up issues such as [NMT: rename NMTUtil::flag to
> NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836)
Gerard Ziemski has updated the pull request incrementally with 308 additional
commits since the last revision:
- undo MEMFLAGS to MemType
- 8339233: Test javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java#id
failed: Button renderings are different after window resize
Reviewed-by: honkar
- 8338924: C1: assert(0 <= i && i < _len) failed: illegal index 5 for length 5
Co-authored-by: Dean Long
Reviewed-by: kvn, thartmann
- 8339492: StackMapDecoder::writeFrames makes lots of allocations
Reviewed-by: liach, redestad, jwaters, asotona
- 8332901: Select{Current,New}ItemTest.java for Choice don't open popup on
macOS
Move SelectCurrentItemTest.java to java/awt/Choice/SelectItem/.
Move SelectNewItemTest.java to java/awt/Choice/SelectItem/.
Use latches to control test flow instead of delays.
Encapsulate the common logic in SelectCurrentItemTest.
Provide overridable checkXXX() methods to modify conditions.
Provide an overridable method which defines where to click
in the choice popup to select an item.
Reviewed-by: honkar, prr, dnguyen
- 8339148: Make os::Linux::active_processor_count() public
Reviewed-by: dholmes, jwaters
- 8339112: Move JVM Klass flags out of AccessFlags
Reviewed-by: matsaave, cjplummer, dlong, thartmann, yzheng
- 8336860: x86: Change integer src operand for CMoveL of 0 and 1 to long
Reviewed-by: epeter, chagedorn, shade, qamai, jbhateja
- 8325679: Optimize ArrayList subList sort
Reviewed-by: liach
- 8339131: Remove rarely-used accessor methods from Opcode
Reviewed-by: asotona
- ... and 298 more: https://git.openjdk.org/jdk/compare/9665d7f7...6d6d70e9
-
Changes:
- all: https://git.openjdk.org/jdk/pull/20497/files
- new: https://git.openjdk.org/jdk/pull/20497/files/9665d7f7..6d6d70e9
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=20497&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=20497&range=00-01
Stats: 40412 lines in 1372 files changed: 23818 ins; 9766 del; 6828 mod
Patch: https://git.openjdk.org/jdk/pull/20497.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20497/head:pull/20497
PR: https://git.openjdk.org/jdk/pull/20497
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Mon, 2 Sep 2024 04:03:28 GMT, David Holmes wrote: > FWIW as I recall the suggestion to include NMT in the name in some form was > to make it clear that these kinds of parameter, which appear all over the > place, are needed because of NMT and are not inherently part of whatever API > they appear in. Whether that happens via a namespace, a nested enum, or a > simple prefix, I don't really care except to say that anything that can then > result in dropping the NMT in the source code (e.g. via a using directive) > completely defeats the purpose of having it in the first place. So if there > is no good answer here than I guess we just drop NMT from the name. Kim and Stefan said that any consideration of using namespace would require additional discussion. And almost everyone dislikes adding the `NMT_` prefix. I feel like we achieved (imperfect) agreement that allows us to proceed with a cleanup using `MemTag` as the new type name to replace `MEMFLAGS`, which I hope everyone agrees is an improvement. I am almost done with that name change and I see it as a significant improvement worthwhile of this effort, with anything else that can be handled in followups, however, if you feel strongly that we should discuss the full topic right now, before proceeding, please let it be known here. Personally I just wanted to cleanup `MEMFLAGS` and related `flag(s)` names that we used in very inconsistent matter. - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2327019788
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) Assuming for a second that we agree on the name of new type ( for now `MemTag`). Are we sure we want to combine that rename `MEMFLAGS` --> `MemTag` with changing the names of parameters and variables around this change at the same time? This is exactly what I initially did, but after I took a 2nd look I got worried about how big it looked. Just for a reference these are my changes I am talking about, which I abandoned: https://openjdk.github.io/cr/?repo=jdk&pr=20472&range=00 - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2321955298
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Thu, 29 Aug 2024 16:17:09 GMT, Stefan Karlsson wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemType` is much more suitable name. >> >> There is a bunch of other related cleanup that we can do, but I will leave >> for follow up issues such as [NMT: rename NMTUtil::flag to >> NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) > > I much prefer to see MemType, but I'm warming up to NMTCategory. > > - MemType: Succinct - matches part of the code (E.g. the mt in mtGC) > - MemTypeFlag: Too many words for my preference. > - NMTCat: Meuw. :) > - NMTCategory: Parts of the code call these categories, so I'm not entirely > against this. > - NMTGroup: "Group" is a new name for this that currently isn't reflected at > all in the code. > - NMT_MemType: I think we should try get rid of names using this style. > - NMT::MemType: The `::` makes all function declarations noisier for very > little benefit, IMO. > I continue to mostly agree with @stefank. > > I think this name shouldn't be considered in isolation. There are already a > bunch of "NMT_" prefixed names. That's the common idiom for things like this > (often (maybe even usually?) without the "_"). Why are we proposing to adopt > a new style. (For just this? That would be weird. Or more broadly? That > certainly needs more discussion.) It is precisely because we already are using `NMT_TrackingStackDepth` and `NMT_TrackingLevel` (and the fact that MemType by itself was too general) that I suggested we adopt `NMT::` Yes, it would be all static class. Why not all lower letter `nmt::`? Again, because we already use `NMT_` elsewhere. My plan was later to switch from `NMT_` to `NMT::` for all of them. We can do `nmt::` too. So how about `MemTag`, `nmt::MemTag` or `NMT::MemTag`? - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2319470647
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) After some more internal discussion `MemTag` and `NMT::MemTag` were suggested. - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2319445188
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) I'm planning on doing only the renaming of MEMFLAGS to NMT::MemType in this fix. The renaming of parameters and variables I will leave to a followup. I will start with NMT first and per Stefan's suggestion I am thinking of using `mt` for the variables/parameters names. - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2318112256
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) Taking Dan's feedback into account we have: || MemType | MemTypeFlag | NMTCat | NMTGroup | NMT_MemType | NMT::MemType | | :---: | :---: | :---: | :---: | :---:| :---: | :---:| | gerard | 1 | 0 | 0 | 0| 2 | 3| | David | 0 | 3 | 0 | 0| 1(NMTMemType) | 2| | Thomas | 0 | 0 | 2 | 3| 0 | 0| | Johan | 0 | 0 | 2(NMTCategory) | 3| 0 | 0| | Afshin | 0 | 3 | 0 | 0| 0 | 2| | Coleen | 0 | 0 | 0 | 0| 0 | 3| | Dan| 0 | 0 | 0 | 0| 2(NMTMemType) | 3| || 1 | 6 | 4 | 6| 5 | 13 | `NMT::MemType` it is. Sorry, if your choice hasn't made it. I am surprised how wide the spectrum of choices are, this took a while to find a consensus. Thank you everyone, who participated! - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2318083570
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) Taking Dan's feedback into account we have: - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2318039929
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Thu, 15 Aug 2024 07:59:21 GMT, Kim Barrett wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemType` is much more suitable name. >> >> There is a bunch of other related cleanup that we can do, but I will leave >> for follow up issues such as [NMT: rename NMTUtil::flag to >> NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) > > The suggestion of "nmtGC" from @daholmes initially looked somewhat appealing > to me. But that then suggests "NMTType" or (better?) "NMTGroup" or something > like that. But I don't much like the look of or typing those acronyms. (Note > that NMTGroup is HotSpot style, not NmtGroup.) > > So I'm still preferring "MemType". @kimbarrett @stefank Would you like to name your nominees? As last voters you have quite weight behind your choices... || MemType | MemTypeFlag | NMTCat | NMTGroup | NMT_MemType | NMT::MemType | | :---: | :---: | :---: | :---: | :---:| :---: | :---:| | gerard | 1 | 0 | 0 | 0| 2 | 3| | David | 0 | 3 | 0 | 0| 1(NMTMemType) | 2| | Thomas | 0 | 0 | 2 | 3| 0 | 0| | Johan | 0 | 0 | 2(NMTCategory) | 3| 0 | 0| | Afshin | 0 | 3 | 0 | 0| 0 | 2| | Stefan | ? | ? | ? | ?| ? | ?| | Kim| ? | ? | ? | ?| ? | ?| | Coleen | 0 | 0 | 0 | 0| 0 | 3| || 1 | 6 | 4 | 6| 3 | 10 | - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2315814746
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Thu, 15 Aug 2024 06:31:14 GMT, David Holmes wrote: >>> I agree with @tstuefe here. MemFlag and MemType sound far too general when >>> this is NMT specific. >> >> Yes, it is not very specific, but it also not hard to learn and then know >> what this type is all about. >> >>> My preference to keep the "flags" part of the type was to avoid needing to >>> rename many parameters. The usage of MEMFLAGS flags is quite extensive. I >>> would not want to see a partial approach here where we end up with a >>> non-flag type name but a flag variable name. >> >> I think we should rename all the 'flags' variables in the same change. >> >>> NMTTypeFlag would I hope satisfy Thomas's requirement and avoid the need to >>> do variable renames. >> >> * To me, that's really not an appealing name for a type that is going to be >> used by all parts of the HotSpot code base. I much more prefer a shorter >> name that is easy on the eyes, then a longer and more specific name that is >> an eyesore. >> >> * And even as a longer name, it doesn't tell what it is going to be used >> for. What is a Native Memory Tracker Type Flag? >> >> * I don't want us to select a bad name so that we don't have to change the >> variable names. >> >> * Whatever we choose we also need to consider the mt prefix of things like >> mtGC, mtClass, etc. >> >> With all that said, I hope it is clear that we various reviewers have >> different opinions around this and that we don't integrate this before we >> have some kind of consensus about the way forward with this. > >> What is a Native Memory Tracker Type Flag? > > It is a flag telling us the type of native memory being tracked. > >> Whatever we choose we also need to consider the mt prefix of things like >> mtGC, mtClass, etc. > > And what does that stand for: memory type? memory tracker? Arguably they > should have been nmtGC etc. > >> I think we should rename all the 'flags' variables in the same change. > > Okay. That's a big change but I'd prefer it to any half-way measures. @dholmes-ora @tstuefe @stefank @kimbarrett @afshin-zafari @jdksjolen hi all, I would like to see if we can give this another go, now that we got some time to sleep on it. How about this - I created a table with some name candidates, so everyone can see where everyone else is. We all can choose 3 candidates that we can rank 1, 2 and 3. At the end we tabulate the answer and the one with highest score wins? | developer | MemType | MemTypeFlag | NMTCat | NMTGroup | NMT_MemType | NMT::MemType | | :---: | :---: | :---: | :---: | :---: | :---: | :---: | | gerard | 1 | 0 | 0 | 0 | 2 | 3 | | David | ? | ? | ? | ? | ? | ? | | Thomas | ? | ? | ? | ? | ? | ? | | Johan | ? | ? | ? | ? | ? | ? | | Afshin | ? | ? | ? | ? | ? | ? | | Stefan | ? | ? | ? | ? | ? | ? | | Kim| ? | ? | ? | ? | ? | ? | - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2313584428
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) We already have `NMT_TrackingLevel` in `nmtCommon.hpp`, so we could do `NMT_MemType` ? We could also add NMT class and then have `NMT::MemType`? Note that as Stefan pointed out, we refer to it as memory type in memflags.hpp Moreover, we use 'mt' in the actual enumerated names as in `mtNone` for example. - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2291520592
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 14 Aug 2024 06:54:22 GMT, Stefan Karlsson wrote: > > Is everyone OK with MemTypeFlag? > > It's quite unfortunate to have a three-word type for something this prolific > in our code base. Why not go with `MemType` and change variable names from > `flag` to `mt`? > > ``` > static char* map_memory_to_file(size_t size, int fd, MEMFLAGS flag = > mtNone); > ``` > > would then become: > > ``` > static char* map_memory_to_file(size_t size, int fd, MemType mt = mtNone); > ``` My initial choice was exactly that, but then I backed-off from renaming the arguments, because how big and intrusive the change it seemed. David seems to prefer `MemTypeFlag`, so that we don't have to rename all the arguments and I see a point in that, but it wouldn't be my first choice. Thomas seems to prefer `NMTCat` that I just don't like much, despite that it has NMT prefix in it, for some reason. If we could find a compromise that we all can live with, despite it not being exactly what every single person wants, then that would be great. We could this in separate steps: Initial effort (this fix): we rename `MEMFLAGS` to `MemType` Follow up effort(s): we either rename all arguments in one big push (intrusive) or we do it a file, or related files (like NMT) together at a time in a followup(s) or whenever we are in the file with some related fix. Eventually we would get there, which is better than what we have right now IMHO. Is this a reasonable compromise to everyone? - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2289134648
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Tue, 13 Aug 2024 01:16:45 GMT, David Holmes wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemType` is much more suitable name. >> >> There is a bunch of other related cleanup that we can do, but I will leave >> for follow up issues such as [NMT: rename NMTUtil::flag to >> NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) > > MemType still makes all the "flag" variable names look weird IMO. @dholmes-ora @tstuefe @jdksjolen Is everyone OK with MemTypeFlag? - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2286658061
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Thu, 8 Aug 2024 04:47:18 GMT, David Holmes wrote: >> Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemType` is much more suitable name. >> >> There is a bunch of other related cleanup that we can do, but I will leave >> for follow up issues such as [NMT: rename NMTUtil::flag to >> NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) > > If you called it `MemTypeFlag` - which to me still suggests > mutually-exclusive values - then you would not need to rename all the > variables with "flag" in their name later. @dholmes-ora @tstuefe @jdksjolen Where are we here? I have renamed MEMFLAGS to MemType. Is this fine, or do you wish to see a different way to handle this? - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2284784541
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) The current proposed fix renames from MEMFLAGS to MemType, other proposed names are: - MemTypeFlag - NMTCategory - NMTCat I also like: - NMTMemType Any other suggestions? I don't think I will be following up with argument renaming after this, though, whatever the final name we agree on... - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2278781358
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) We could do: `#define MEMFLAGS MemType` to help out with backports, and leave it up to the runtime/compiler/gc teams when to pull the trigger to switch over when they are ready, if that helps? Any backport would have to deal with this change to a degree though unfortunately. I would want this change in runtime/nmt at the minimum, though personally I would prefer if we adopted it everywhere as in my proposed fix. - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2278776049
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Thu, 8 Aug 2024 04:47:18 GMT, David Holmes wrote: > If you called it `MemTypeFlag` - which to me still suggests > mutually-exclusive values - then you would not need to rename all the > variables with "flag" in their name later. Hmm, not a bad idea. Are there any other opinions? - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2276354395
RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. `MEMFLAGS` implies that we can use more than one at the same time, but those are exclusive values, so `MemType` is much more suitable name. There is a bunch of other related cleanup that we can do, but I will leave for follow up issues such as [NMT: rename NMTUtil::flag to NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) - Commit messages: - rename MEMFLAGS to MemType Changes: https://git.openjdk.org/jdk/pull/20497/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20497&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337563 Stats: 502 lines in 100 files changed: 1 ins; 0 del; 501 mod Patch: https://git.openjdk.org/jdk/pull/20497.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20497/head:pull/20497 PR: https://git.openjdk.org/jdk/pull/20497
Withdrawn: 8337563: NMT: rename MEMFLAGS to MemFlag
On Mon, 5 Aug 2024 19:07:08 GMT, Gerard Ziemski wrote: > Please review this NMT cleanup change that renames `MEMFLAGS` to `MemType`. > > To keep this change down I decided not to do any other cleanups as part of > this fix. They will be handled in followup issues, such as [NMT: rename > NMTUtil::flag to NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/20472
RFR: 8337563: NMT: rename MEMFLAGS to MemFlag, use consistent name for the argument
Please review this NMT cleanup change that mainly renames `MEMFLAGS` to `MemType`, as well as cleanups the related arguments names. This avoids the inconsistencies and confusion currently on display in our code, where we use `flag`, `flags`, `mem_flag`, `memflags` for the same argument, even in the same file in related APIs. I made sure to change the related copyright years in all touched files. This change is rather simple - we are just renaming names, but it touches 103 files unfortunately. I did not rename any `NMTUtil::flag` (ex. `NMTUtil::flag_to_index()` -> `NMTUtil::type_to_index()`) to keep the amount of code changes smaller. Those APIs should be renamed, but I filed a followup issue for this [NMT: rename NMTUtil::flag to NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) There are also further cleanup opportunities here (ex. renaming internal fields from `_memflags` to `_mem_type`), but again these can be addressed in followup issues. - Commit messages: - undo incorrect rename - undo incorrect rename - more flag cleanup - rename MEMFLAGS to MemType and any related arguments that make sense Changes: https://git.openjdk.org/jdk/pull/20472/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20472&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337563 Stats: 850 lines in 103 files changed: 1 ins; 0 del; 849 mod Patch: https://git.openjdk.org/jdk/pull/20472.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20472/head:pull/20472 PR: https://git.openjdk.org/jdk/pull/20472
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v7]
On Wed, 31 Jan 2024 08:01:15 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Minor - fix alignment Marked as reviewed by gziemski (Committer). - PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1854147646
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v5]
On Tue, 30 Jan 2024 16:17:58 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > assert that we don't pass values higher than INT_MAX to close() This fix looks very good, thank you @jaikiran ! - Marked as reviewed by gziemski (Committer). PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1851797639
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v4]
On Tue, 30 Jan 2024 16:15:17 GMT, Jaikiran Pai wrote: >>> If we are going to check close for errors then we will have to know for >>> certain we are only trying to close open fd's, and we will have to deal >>> with EINTR. I would think this is a "best effort" to close open FD's and >>> not something we have to care about in detail. >> >> Right, EINTR would need to be ignored as it would be wrong to restart. I'm >> scratching my head as to why this code would even do with EIO or if it even >> possible here. > > Looking at man close, EIO gets returned if: >> [EIO] A previously-uncommitted write(2) encountered an >> input/output error. > > If I understand this `launch=` option correctly for the jdwp agent, then > the arbitrary command gets launched very early in the lifecycle of the JVM. > So in theory, I think there shouldn't be too many file descriptors that the > JVM has opened so far and the chances of it having started writing to some of > those opened file descriptors (other than standard out/err) perhaps is slim? > But that's just a guess. Probably not a strong one too, given that if the JVM > was launched with any other agent along with jdwp, then it's unknown if the > other agent would have opened any file descriptors for write. > > My understanding of these close() calls after fork() in this code, matches > David's - I think it's a best effort attempt. So perhaps ignoring such > failures, like we do now, is OK? Ignoring errors just doesn't seem like a good strategy. This fix right here that we are doing doesn't have to deal with making this code more robust, as long as we follow up in another issue. For that, however, I filed JDK-8324979, and that frees us up to move on with this fix. - PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1471566159
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v5]
On Tue, 30 Jan 2024 16:17:58 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > assert that we don't pass values higher than INT_MAX to close() I filed https://bugs.openjdk.org/browse/JDK-8324979 as a followup to make sure we can get this code as robust as possible. - PR Comment: https://git.openjdk.org/jdk/pull/17588#issuecomment-1917424839
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v2]
On Sat, 27 Jan 2024 01:18:09 GMT, Jaikiran Pai wrote:
>> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 129:
>>
>>> 127: * and assume all were opened for the parent process and
>>> 128: * copied over to this child process. we close them all */
>>> 129: const rlim_t max_fd = sysconf(_SC_OPEN_MAX);
>>
>> Why not use `int`, like the original code, instead of `rlim_t` - as per man
>> page `close()` expects `int`, not `rlim_t`, ex:
>>
>>
>> const int max_fd = (int)sysconf(_SC_OPEN_MAX);
>> JDI_ASSERT(max_fd != -1);
>> int fd;
>> /* leave out standard input/output/error file descriptors */
>> for (fd = STDERR_FILENO + 1; fd < max_fd; fd++) {
>> (void)close(fd);
>> }
>
> Hello Gerard, my understanding is that the limit value configured may exceed
> the int range. I wanted to avoid the overflow by casting it to int in such
> cases. I had noticed close() takes an int, but I couldn't think of any other
> way of avoiding the overflow at this place.
>
> In the JVM parent process we do however limit it to INT_MAX. So maybe I
> should assume that it will be an int cast it to int here, like you suggest,
> and add a code comment explaining this? Does that sound OK?
That is fine, but in that case we should also do:
`JDI_ASSERT(max_fd <= INT_MAX);`
in case `sysconf(_SC_OPEN_MAX)` returns value greater than `INT_MAX`, since
`close()` only accepts `int`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1469976093
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v4]
On Mon, 29 Jan 2024 10:00:37 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to address
>> https://bugs.openjdk.org/browse/JDK-8324668?
>>
>> This change proposes to fix the issue in jdwp where when launching a child
>> process (for the `launch=` option), it iterates over an extremely large
>> number of file descriptors to close each one of those. Details about the
>> issue and the proposed fixed are added in a subsequent comment in this PR
>> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1,
>> tier2 and tier3 testing is currently in progress with this change.
>
> Jaikiran Pai has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - add a log message to help debuggability
> - improve code comments
> - David's review - use standard isdigit instead of custom isAsciiDigit
Again, the changes make the code better already, I just want to make sure take
this opportunity to polish this code really well...
src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 108:
> 106: if (isdigit(dirp->d_name[0]) &&
> 107: (fd = strtol(dirp->d_name, NULL, 10)) >= from_fd) {
> 108: (void)close(fd);
I'd really, really like to check `close()` for errors and report any.
src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 138:
> 136: " %d file descriptors sequentially", (max_fd - i +
> 1)));
> 137: for (; i < max_fd; i++) {
> 138: (void)close(i);
Here as well, I'd really like to check for `close()`for errors here and report
any.
And if we find errors here, do we really want to proceed knowing that something
went wrong already?
-
Changes requested by gziemski (Committer).
PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1849357100
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1469984071
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1469979677
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v2]
On Fri, 26 Jan 2024 15:57:57 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > use the right include for rlim_t - Looks good, thank you for debugging this and providing a fix! I did have some questions and feedback, nothing major, just making sure we cover all our bases. - PR Comment: https://git.openjdk.org/jdk/pull/17588#issuecomment-1912438110
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v2]
On Fri, 26 Jan 2024 15:57:57 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > use the right include for rlim_t - src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 112: > 110: } > 111: > 112: closedir(dp); Should this be: `(void)closedir(dp);` to show that we're ignoring the return value? Since this area is pretty mysterious to deal with, should we consider checking whether `closedir()` actually returns am error, and at least be verbal and print a warning, as opposed to silently ignoring it? Printing a warning, might help us diagnose any issue in the future? - PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1467934723
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v2]
On Fri, 26 Jan 2024 15:57:57 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to address
>> https://bugs.openjdk.org/browse/JDK-8324668?
>>
>> This change proposes to fix the issue in jdwp where when launching a child
>> process (for the `launch=` option), it iterates over an extremely large
>> number of file descriptors to close each one of those. Details about the
>> issue and the proposed fixed are added in a subsequent comment in this PR
>> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1,
>> tier2 and tier3 testing is currently in progress with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use the right include for rlim_t -
Have you considered using `fcntl(d, F_SETFD, 1)` instead of the fancy
`closeDescriptors()`?
I haven't tested it myself, but per the man page:
Most of the descriptors can be rearranged with dup2(2) or deleted with close()
before the execve is attempted, but if some of these descriptors will still be
needed if the execve fails, it is necessary to arrange for them to be closed if
the execve succeeds. For this reason, the call “fcntl(d, F_SETFD, 1)” is
provided, which arranges that a descriptor will be closed after a successful
execve;
src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 129:
> 127: * and assume all were opened for the parent process and
> 128: * copied over to this child process. we close them all */
> 129: const rlim_t max_fd = sysconf(_SC_OPEN_MAX);
Why not use `int`, like the original code, instead of `rlim_t` - as per man
page `close()` expects `int`, not `rlim_t`, ex:
const int max_fd = (int)sysconf(_SC_OPEN_MAX);
JDI_ASSERT(max_fd != -1);
int fd;
/* leave out standard input/output/error file descriptors */
for (fd = STDERR_FILENO + 1; fd < max_fd; fd++) {
(void)close(fd);
}
-
Changes requested by gziemski (Committer).
PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1846188473
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1467927052
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v2]
On Fri, 26 Jan 2024 15:57:57 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to address
>> https://bugs.openjdk.org/browse/JDK-8324668?
>>
>> This change proposes to fix the issue in jdwp where when launching a child
>> process (for the `launch=` option), it iterates over an extremely large
>> number of file descriptors to close each one of those. Details about the
>> issue and the proposed fixed are added in a subsequent comment in this PR
>> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1,
>> tier2 and tier3 testing is currently in progress with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use the right include for rlim_t -
src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 135:
> 133: for (; i < max_fd; i++) {
> 134: (void)close(i);
> 135: }
I think I'd prefer to see standard:
rlim_t i;
for (STDERR_FILENO + 1; i < max_fd; i++) {
(void)close(i);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1467922003
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v7]
On Wed, 1 Nov 2023 10:09:40 GMT, Thomas Stuefe wrote: >> Analysts and supporters often use /proc/xx/maps to make sense of the memory >> footprint of a process. >> >> Interpreting the memory map correctly can help when used as a complement to >> other tools (e.g. NMT). There even exist tools out there that attempt to >> annotate the process memory map with JVM information. >> >> That, however, can be more accurately done from within the JVM, at least for >> mappings originating from hotspot. After all, we can correlate the mapping >> information in NMT with the VMA map. >> >> Example output (from a spring petstore run): >> >> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt) >> >> This patch adds the VM annotations for VMAs that are *mmaped*. I also have >> an experimental patch that works with malloc'ed memory, but it is not ready >> yet for consumption and I leave that part of for now. > > Thomas Stuefe has updated the pull request incrementally with three > additional commits since the last revision: > > - Provide both print and dump command > - Feedback Gerard > - Feedbacj Johan LGTM Thank you for doing this! - Marked as reviewed by gziemski (Committer). PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1708486866
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
On Sat, 28 Oct 2023 13:04:05 GMT, Thomas Stuefe wrote:
>> Analysts and supporters often use /proc/xx/maps to make sense of the memory
>> footprint of a process.
>>
>> Interpreting the memory map correctly can help when used as a complement to
>> other tools (e.g. NMT). There even exist tools out there that attempt to
>> annotate the process memory map with JVM information.
>>
>> That, however, can be more accurately done from within the JVM, at least for
>> mappings originating from hotspot. After all, we can correlate the mapping
>> information in NMT with the VMA map.
>>
>> Example output (from a spring petstore run):
>>
>> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt)
>>
>> This patch adds the VM annotations for VMAs that are *mmaped*. I also have
>> an experimental patch that works with malloc'ed memory, but it is not ready
>> yet for consumption and I leave that part of for now.
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix various builds
Just a couple of issues/questions.
This is a very nice feature we're adding. Thank you!
src/hotspot/os/linux/memMapPrinter_linux.cpp line 59:
> 57: void print_OS_specific_details(outputStream* st) const override {
> 58: st->print("%s %s ", _info.prot, _info.offset);
> 59: }
If that's all this is doing, do we really need it?
src/hotspot/os/linux/memMapPrinter_linux.cpp line 83:
> 81: char line[linesize];
> 82: while(fgets(line, sizeof(line), f) == line) {
> 83: line[sizeof(line) - 1] = '\0';
What would happen if the read line is empty, i.e. sizeof(line)==0, can that
happen?
src/hotspot/share/nmt/memMapPrinter.cpp line 79:
> 77: const void* const min = MAX2(from1, from2);
> 78: const void* const max = MIN2(to1, to2);
> 79: return min < max;
I had to rewrite it as:
`return MAX2(from1, from2) < MIN2(to1, to2);`
to make sure I understand it, or better yet, why not have it as a macros?
`#define RANGE_INTERSECT(min, max) (return MAX2(from1, from2) < MIN2(to1, to2))`
MAX2 and MIN2 are macros already and we're not doing anything fancy here.
src/hotspot/share/nmt/memMapPrinter.cpp line 89:
> 87: // NMT deadlocks (ThreadCritical).
> 88: Range* _ranges;
> 89: MEMFLAGS* _flags;
Why did you decide to have two separate memory chunks?
I think I'd have used a struct to hold Range* and MEMFLAGS* and use that struct
in a single array.
-
Changes requested by gziemski (Committer).
PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1704722251
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1377905955
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1376585794
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1377917390
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1377926176
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v4]
On Fri, 27 Oct 2023 12:28:51 GMT, Thomas Stuefe wrote:
>> Analysts and supporters often use /proc/xx/maps to make sense of the memory
>> footprint of a process.
>>
>> Interpreting the memory map correctly can help when used as a complement to
>> other tools (e.g. NMT). There even exist tools out there that attempt to
>> annotate the process memory map with JVM information.
>>
>> That, however, can be more accurately done from within the JVM, at least for
>> mappings originating from hotspot. After all, we can correlate the mapping
>> information in NMT with the VMA map.
>>
>> Example output (from a spring petstore run):
>>
>> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt)
>>
>> This patch adds the VM annotations for VMAs that are *mmaped*. I also have
>> an experimental patch that works with malloc'ed memory, but it is not ready
>> yet for consumption and I leave that part of for now.
>
> Thomas Stuefe has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Fix spelling
> - timeout fuse
> - Feedback Johan
Changes requested by gziemski (Committer).
src/hotspot/share/services/memMapPrinter.cpp line 49:
> 47: // Short, clear, descriptive names for all possible markers. Note that we
> only expect to see
> 48: // those that have been used with mmap. Others I leave at nullptr.
> 49: #define NMTFLAGS_DO(f) \
NMTFLAGS_DO --> NMT_FLAGS_DO to follow the pattern of IK_FLAGS_DO, CM_FLAGS_DO
src/hotspot/share/services/memMapPrinter.cpp line 95:
> 93: };
> 94:
> 95: /// NMT virtual memory
Should we try to put all this NMT flags code into allocation.hpp?
src/hotspot/share/services/memMapPrinter.cpp line 181:
> 179: static uintx safely_get_thread_id(const Thread* t) {
> 180: const OSThread* osth = t->osthread();
> 181: uintx tid = 0;
Leftover?
src/hotspot/share/services/memMapPrinter.cpp line 188:
> 186: }
> 187:
> 188: // Given a region [from, to) that is supposed to represent a thread
> stack,
Need to finish the comment?
src/hotspot/share/services/memMapPrinter.cpp line 192:
> 190:
> 191: for (JavaThreadIteratorWithHandle jtiwh; JavaThread* t = jtiwh.next();
> ) {
> 192: const size_t len = pointer_delta(to, from, 1);
Leftover?
src/hotspot/share/services/memMapPrinter.cpp line 196:
> 194: st->print("(" UINTX_FORMAT " \"%s\")", safely_get_thread_id(t),
> t->name());
> 195: return;
> 196: }
Can we use `HANDLE_THREAD` here instead?
src/hotspot/share/services/memMapPrinter.cpp line 294:
> 292: // the absolute runtime of printing to blocking other VM operations
> too long.
> 293: const jlong timeout_at = os::javaTimeNanos() +
> 294:((jlong)(SafepointTimeoutDelay *
> NANOSECS_PER_MILLISEC) / 2);
Is AbortVMOnSafepointTimeoutDelay/2 timeout useful for real world cases?
I assume the client who would need this feature, would need it for a reason and
that means memory hungry processes?
Instead of blocking VM and having to use the timeout, can we:
A. take a snapshot of NMT virtual memory region and process it without blocking
the VM
B. divide this process into smaller chunks, each of which will be guaranteed to
finish before AbortVMOnSafepointTimeoutDelay triggers
C. temporarily increase AbortVMOnSafepointTimeoutDelay and revert it back
afterwords?
In case A,B we would get back a very close, but not exact view of memory, but
wouldn't this be better than getting back precise, but cut off view?
Case C would give us what the users wants assuming they are OK with waiting a
bit longer?
And adding malloc'ed memory here would be make this issue much worse.
-
PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1702475105
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375002985
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375011085
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375017943
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375020309
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375020664
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375021914
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1375080669
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v4]
On Fri, 27 Oct 2023 12:28:51 GMT, Thomas Stuefe wrote: >> Analysts and supporters often use /proc/xx/maps to make sense of the memory >> footprint of a process. >> >> Interpreting the memory map correctly can help when used as a complement to >> other tools (e.g. NMT). There even exist tools out there that attempt to >> annotate the process memory map with JVM information. >> >> That, however, can be more accurately done from within the JVM, at least for >> mappings originating from hotspot. After all, we can correlate the mapping >> information in NMT with the VMA map. >> >> Example output (from a spring petstore run): >> >> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt) >> >> This patch adds the VM annotations for VMAs that are *mmaped*. I also have >> an experimental patch that works with malloc'ed memory, but it is not ready >> yet for consumption and I leave that part of for now. > > Thomas Stuefe has updated the pull request incrementally with three > additional commits since the last revision: > > - Fix spelling > - timeout fuse > - Feedback Johan I really like this feature, hope the other platforms can be done as well. (I am also really looking forward to seeing how you did the "malloc" version) I think we should set the user expectations correctly and say somewhere that these mappings are only for mmaped memory, ex change: `Memory mappings:` to `Memory mappings (mmaped):` or something like that. These lines look akward next to each other: 0x00070a40 - 0x0008 4123000832 ---p JAVAHEAP 0x7f4e747dd000 - 0x7f4e747e1000 16384 ---p STACK(72821 "AWT-EventQueue-0") 0x563e6decb000 - 0x563e6decc0004096 r--p /home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/bin/java 0x7f4e2600 - 0x7f4e26d3b00013873152 rw-p 1000CDS /home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/lib/server/classes.jsa in my opinion. The majority of entries either have the component or path in the **info** field (except CDS). Could we print it simply as: 0x00070a40 - 0x0008 4123000832 ---p JAVAHEAP 0x7f4e747dd000 - 0x7f4e747e1000 16384 ---p STACK(72821 "AWT-EventQueue-0") 0x563e6decb000 - 0x563e6decc0004096 r--p /home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/bin/java 0x7f4e2600 - 0x7f4e26d3b00013873152 rw-p 1000 CDS=/home/gerard/Desktop/Work/8318636/jdk/build/linux-x86_64-server-fastdebug/images/jdk/lib/server/classes.jsa I find the compact way more aesthetically pleasing and easier to read. Next I'm going to start looking into the implementation... - Changes requested by gziemski (Committer). PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1702252387
Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v4]
On Fri, 27 Oct 2023 12:28:51 GMT, Thomas Stuefe wrote: >> Analysts and supporters often use /proc/xx/maps to make sense of the memory >> footprint of a process. >> >> Interpreting the memory map correctly can help when used as a complement to >> other tools (e.g. NMT). There even exist tools out there that attempt to >> annotate the process memory map with JVM information. >> >> That, however, can be more accurately done from within the JVM, at least for >> mappings originating from hotspot. After all, we can correlate the mapping >> information in NMT with the VMA map. >> >> Example output (from a spring petstore run): >> >> [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt) >> >> This patch adds the VM annotations for VMAs that are *mmaped*. I also have >> an experimental patch that works with malloc'ed memory, but it is not ready >> yet for consumption and I leave that part of for now. > > Thomas Stuefe has updated the pull request incrementally with three > additional commits since the last revision: > > - Fix spelling > - timeout fuse > - Feedback Johan Taking a look... - PR Comment: https://git.openjdk.org/jdk/pull/16301#issuecomment-1783130267
