Re: RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v3]
On Wed, 8 Jun 2022 12:09:27 GMT, Severin Gehwolf wrote: >> Please review this cleanup change in the cgroup subsystem which used to use >> hard-coded stack allocated >> buffers for concatenating strings in memory. We can use `stringStream` >> instead which doesn't have the issue >> of hard-coding maximum lengths (and related checks) and makes the code, >> thus, easier to read. >> >> While at it, I've written basic `gtests` verifying current behaviour (and >> the same for the JDK code). From >> a functionality point of view this is a no-op (modulo the one bug in the >> substring case which seems to be >> there since day one). I'd prefer if we could defer any change in >> functionality to a separate bug as this is >> meant to only use stringStream throughout the cgroups code. >> >> Testing: >> - [x] Container tests on Linux x86_64 cgroups v1 and cgroups v2 >> - [x] Added tests, which I've verified also pass before the stringStream >> change >> >> Thoughts? > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Remove fix for substring match Just came back from vacation. LGTM. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/8969
Re: RFR: 8288222: ProblemList serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java
On Fri, 10 Jun 2022 15:21:34 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > serviceability/jvmti/vthread/VThreadNotifyFramePopTest/VThreadNotifyFramePopTest.java. Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.org/jdk19/pull/4
Re: RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v2]
On Tue, 7 Jun 2022 12:42:26 GMT, Severin Gehwolf wrote:
>> Please review this cleanup change in the cgroup subsystem which used to use
>> hard-coded stack allocated
>> buffers for concatenating strings in memory. We can use `stringStream`
>> instead which doesn't have the issue
>> of hard-coding maximum lengths (and related checks) and makes the code,
>> thus, easier to read.
>>
>> While at it, I've written basic `gtests` verifying current behaviour (and
>> the same for the JDK code). From
>> a functionality point of view this is a no-op (modulo the one bug in the
>> substring case which seems to be
>> there since day one). I'd prefer if we could defer any change in
>> functionality to a separate bug as this is
>> meant to only use stringStream throughout the cgroups code.
>>
>> Testing:
>> - [x] Container tests on Linux x86_64 cgroups v1 and cgroups v2
>> - [x] Added tests, which I've verified also pass before the stringStream
>> change
>>
>> Thoughts?
>
> Severin Gehwolf has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains six additional
> commits since the last revision:
>
> - Merge branch 'master' into jdk-8287007-string-stream
> - Add cgroups v2 java test
> - use stringStream in cgroups v2
> - Add gtest for cgroups v2 code path
>
>Also fixes the bug when cgroup path is '/'.
> - 8287007: [cgroups] Consistently use stringStream throughout parsing code
> - 8287007: [cgroups] Consistently use stringStream throughout parsing code
Changes requested by iklam (Reviewer).
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 54:
> 52: } else {
> 53: char *p = strstr(cgroup_path, _root);
> 54: if (p != NULL && p == cgroup_path) {
I think this change should be done in a separate bug, because it will cause the
`if` block to be executed. Previously the `if` block is never executed (unless
`cgroup_path == _root` ??, but then it will just set `_path` to the same string
as `_mount_point`) -- so we don't even know if this change of behavior might do
something harmful.
-
PR: https://git.openjdk.java.net/jdk/pull/8969
Re: RFR: 8287135: Calculation of jmm_GetMemoryUsage is wrong [v3]
On Tue, 7 Jun 2022 10:07:08 GMT, Yi Yang wrote:
>> It seems that calculation of
>> MemoryMXBean.getNonHeapMemoryUsage(jmm_GetMemoryUsage) is wrong.
>>
>> Currently,
>> `NonHeapUsage=CodeCache+Metaspace(ClassTypeSpace+NonClassTypeSpace)+CompressedClassSpace(ClassTypeSpace)`
>>
>> ==> CodeHeap 'non-nmethods' 1532544 (Used)
>> ==> CodeHeap 'profiled nmethods' 0
>> ==> CodeHeap 'non-profiled nmethods' 13952
>> ==> Metaspace 506696
>> ==> Compressed Class Space 43312
>> init = 7667712(7488K) used = 2096504(2047K) committed = 8454144(8256K) max =
>> -1(-1K)
>>
>> In this way, getNonHeapMemoryUsage is larger than it ought to be, it should
>> be `NonHeapUsage = CodeCache + Metaspace`.
>
> Yi Yang has updated the pull request incrementally with one additional commit
> since the last revision:
>
> update
@tstuefe could you take a look at the test changes. Since we can no longer
query the compressed class space individually, I think the tests may become
more lenient than before. For example,
memoryUsageSmallComp/TestDescription.java uses
`MemoryUsageTest::checkForNotGrowing()` which monitors the used bytes in
`MetaspaceBaseGC::pool.getUsage().getUsed()`
- Before this PR, it checks that the usage of the compressed klass space
doesn't grow.
- After this PR, it will allow the compresed klass space to grow, as long as
the "other" meta space shrinks by a similar amount.
Is this OK? Or should we add a new whitebox API to query the compressed vs meta
space?
src/hotspot/share/services/management.cpp line 743:
> 741: } else {
> 742: // Calculate the memory usage by summing up the pools.
> 743: // NonHeapUsage = CodeHeaps + Metaspace
I think the new comments in this file can be removed.
test/hotspot/jtreg/vmTestbase/metaspace/gc/MemoryUsageTest.java line 141:
> 139: System.err.println("Usage: ");
> 140: System.err.println("java [-Xms..] [-XX:MetaspaceSize=..]
> [-XX:MaxMetaspaceSize=..] \\");
> 141: System.err.println("" +
> MemoryUsageTest.class.getCanonicalName());
This method can be deleted since it's no longer used.
-
PR: https://git.openjdk.java.net/jdk/pull/8831
Re: RFR: 8287663 Add a regression test for JDK-8287073
On Mon, 6 Jun 2022 23:07:06 GMT, Ioi Lam wrote: > We should try to consolidate these test cases to improve maintainability. I filed [JDK-8287185](https://bugs.openjdk.org/browse/JDK-8287185) - PR: https://git.openjdk.java.net/jdk/pull/8993
Re: RFR: 8287663 Add a regression test for JDK-8287073
On Thu, 2 Jun 2022 14:32:28 GMT, Severin Gehwolf wrote: > This adds a regression test for a recent fix (JDK-8287073). I've restructured > the linux specific JDK code to call a separate static function to enable this > test. It'll help future tests too. > > Testing: > - [x] Container tests continue to pass + GHA > - [x] New regression test fails prior the code fix of JDK-8287073 and passes > with it Not specific to this PR, but we have a general problem with lots of duplication between these two files. E.g., https://github.com/openjdk/jdk/blob/a50b06e85124f61b5133189a2a2e461753d5d9e7/test/hotspot/jtreg/containers/cgroup/CgroupSubsystemFactory.java#L132-L143 https://github.com/openjdk/jdk/blob/a50b06e85124f61b5133189a2a2e461753d5d9e7/test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java#L130-L143 We should try to consolidate these test cases to improve maintainability. - PR: https://git.openjdk.java.net/jdk/pull/8993
Re: RFR: 8287663 Add a regression test for JDK-8287073
On Thu, 2 Jun 2022 14:32:28 GMT, Severin Gehwolf wrote: > This adds a regression test for a recent fix (JDK-8287073). I've restructured > the linux specific JDK code to call a separate static function to enable this > test. It'll help future tests too. > > Testing: > - [x] Container tests continue to pass + GHA > - [x] New regression test fails prior the code fix of JDK-8287073 and passes > with it LGTM. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8993
Re: RFR: 8287135: Calculation of jmm_GetMemoryUsage is wrong
On Fri, 27 May 2022 07:22:11 GMT, Thomas Stuefe wrote:
> > If we remove `CompressedClassSpace`, we can only
> > `getMemoryPool("Metaspace")`. Although metaspace is not baked in the
> > specification, IMHO it's easier for developers to understand what is
> > `metaspace` compared to the concepts of `Non Class Space` and `Compressed
> > Class Space`.
>
> I personally think that would be totally fine.
I also vote for removing `CompressedClassSpacePool`
-
PR: https://git.openjdk.java.net/jdk/pull/8831
Re: RFR: 8287135: Calculation of jmm_GetMemoryUsage is wrong
On Fri, 27 May 2022 05:31:24 GMT, David Holmes wrote: > The basic problem is that we have two non-heap pools: > > * `MetaspacePool` > > * consists of `ClassType` and `NonClassType` parts > * `CompressedKlassSpacePool` > > but the `CompressedKlassSpacePool` is actually the "ClassType" part of the > `MetaspacePool`! > > I think the right fix is to just convert the `MetaspacePool` into > `NonClassMetaspacePool` and only report the non-class values. > > AFAICS this will only be visible via the MXBean. When CDS is enabled, the CompressedKlassSpacePool actually contains more than Klass objects. It can contain other metadata such as Method, ConstantPool, etc. So calling these pools "class" and "non-class" is not 100% correct. Is there any reason for separating the CompressedKlassSpacePool from other metadata? I know in the past it was possible to run out of spaces in CompressedKlassSpace, so there might be a reason to monitor it separately? - PR: https://git.openjdk.java.net/jdk/pull/8831
Re: RFR: 8287135: Calculation of jmm_GetMemoryUsage is wrong
On Mon, 23 May 2022 07:28:41 GMT, Yi Yang wrote:
> It seems that calculation of
> MemoryMXBean.getNonHeapMemoryUsage(jmm_GetMemoryUsage) is wrong.
>
> Currently,
> `NonHeapUsage=CodeCache+Metaspace(ClassTypeSpace+NonClassTypeSpace)+CompressedClassSpace(ClassTypeSpace)`
>
> ==> CodeHeap 'non-nmethods' 1532544 (Used)
> ==> CodeHeap 'profiled nmethods' 0
> ==> CodeHeap 'non-profiled nmethods' 13952
> ==> Metaspace 506696
> ==> Compressed Class Space 43312
> init = 7667712(7488K) used = 2096504(2047K) committed = 8454144(8256K) max =
> -1(-1K)
>
> In this way, getNonHeapMemoryUsage is larger than it ought to be, it should
> be `NonHeapUsage = CodeCache + Metaspace`.
src/hotspot/share/services/management.cpp line 753:
> 751: for (int i = 0; i < MemoryService::num_memory_pools(); i++) {
> 752: MemoryPool* pool = MemoryService::get_memory_pool(i);
> 753: if (pool->is_codeheap() || pool->is_metaspace()) {
Our only special case is that all the memory reported by
`CompressedKlassSpacePool` are already reported by `MetaspacePool`, so
shouldn't we do this:
if (pool->is_non_heap() && !pool->is_compressed_klass_space()) {
// skip CompressedKlassSpacePool since its memory is already reported by
// MetaspacePool
-
PR: https://git.openjdk.java.net/jdk/pull/8831
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]
On Thu, 19 May 2022 19:59:18 GMT, Ioi Lam wrote:
>>> What happens if cgroup_path is "/foo/bar" and _root is "/fo"?
>>
>> With a mount path of `/bar` this ends up being `/bar/o/bar`. Pretty strange,
>> but then again it's a bit of a contrived example as those paths come from
>> `/proc` parsing. Anyway, this is code that got added with
>> [JDK-8146115](https://bugs.openjdk.java.net/browse/JDK-8146115). It's not
>> something I've written and to be honest, I'm not sure this branch is needed,
>> but I didn't want to change the existing behaviour with this patch. I have
>> no more insight than you in terms of why that approach has been taken.
>>
>>> Maybe this block should be combined with the new `else` block you're adding?
>>
>> Maybe, but I'm not sure if it would break something.
>>
>>> However, the behavior seems opposite between these two blocks of code:
>>>
>>> The upper block: _root is a prefix of cgroup_path, we take the **tail** of
>>> cgroup_path
>>>
>>> ```
>>> TestCase substring_match = {
>>> "/sys/fs/cgroup/memory", // mount_path
>>> "/user.slice/user-1000.slice", // root_path
>>> "/user.slice/user-1000.slice/user@1001.service", // cgroup_path
>>> "/sys/fs/cgroup/memory/user@1001.service" // expected_path
>>> };
>>> ```
>>
>> Yes. Though, I cannot comment on why that has been chosen. It's been there
>> since day one :-/
>>
>>> The lower block: The beginning of _root is a prefix of cgroup_path, we take
>>> the **head** of cgroup_path
>>>
>>> ```
>>> TestCase prefix_matched_cg = {
>>> "/sys/fs/cgroup/memory", // mount_path
>>> "/user.slice/user-1000.slice/session-50.scope",// root_path
>>> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
>>> "/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path
>>> };
>>> ```
>>>
>>> I find the behavior hard to understand. I think the rules (and reasons)
>>> should be added to the comment block above the function.
>>
>> The reason why I've gone down the path of adding the head of cgroup_path is
>> because of this document (in conjunction to what the user was observing on
>> an affected system):
>> https://www.freedesktop.org/wiki/Software/systemd/ControlGroupInterface/
>>
>> The user was observing paths as listed in the test:
>>
>> "/user.slice/user-1000.slice/session-50.scope",// root_path
>> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
>>
>> This very much looks like systemd managed. Given that and knowing that
>> systemd adds processes into `scopes` or `services` and groups them via
>> `slices` and also knowing that cgroups are hierarchical (i.e. limits of
>> `/foo/bar` also apply to `/foo/bar/baz`), it seems likely that if there are
>> any limits, then it'll be on `/user.slice/user-1000.slice` within the
>> mounted controller. Unfortunately, I'm not able to reproduce this myself.
>
> I am wondering if the problem is this:
>
> We have systemd running on the host, and a different copy of systemd that
> runs inside the container.
>
> - They both set up `/user.slice/user-1000.slice/session-??.scope` within
> their own file systems
> - For some reason, when you're looking inside the container,
> `/proc/self/cgroup` might use a path in the containerized file system whereas
> `/proc/self/mountinfo` uses a path in the host file system. These two paths
> may look alike but they have absolutely no relation to each other.
>
> I have asked the reporter for more information:
>
> https://gist.github.com/gaol/4d96eace8290e6549635fdc0ea41d0b4?permalink_comment_id=4172593#gistcomment-4172593
>
> Meanwhile, I think the current method of finding "which directory under
> /sys/fs/cgroup/memory controls my memory usage" is broken. As mentioned
> about, the path you get from `/proc/self/cgroup` and `/proc/self/mountinfo`
> have no relation to each other, but we use them anyway to get our answer,
> with many ad-hoc methods that are not documented in the code.
>
> Maybe we should do this instead?
>
> - Read /proc/self/cgroup
> - Find the `10:memory:` line
> - If `/sys/fs/cgroup/memory//tasks` contains my PID, this is the path
> - Other
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]
On Thu, 19 May 2022 09:06:18 GMT, Severin Gehwolf wrote:
>> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 63:
>>
>>> 61: } else {
>>> 62: char *p = strstr(cgroup_path, _root);
>>> 63: if (p != NULL && p == cgroup_path) {
>>
>> What happens if cgroup_path is "/foo/bar" and _root is "/fo"?
>>
>> Maybe this block should be combined with the new `else` block you're adding?
>> However, the behavior seems opposite between these two blocks of code:
>>
>> The upper block: _root is a prefix of cgroup_path, we take the **tail** of
>> cgroup_path
>>
>>
>> TestCase substring_match = {
>> "/sys/fs/cgroup/memory", // mount_path
>> "/user.slice/user-1000.slice", // root_path
>> "/user.slice/user-1000.slice/user@1001.service", // cgroup_path
>> "/sys/fs/cgroup/memory/user@1001.service" // expected_path
>> };
>>
>>
>> The lower block: The beginning of _root is a prefix of cgroup_path, we take
>> the **head** of cgroup_path
>>
>>
>> TestCase prefix_matched_cg = {
>> "/sys/fs/cgroup/memory", // mount_path
>> "/user.slice/user-1000.slice/session-50.scope",// root_path
>> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
>> "/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path
>> };
>>
>>
>> I find the behavior hard to understand. I think the rules (and reasons)
>> should be added to the comment block above the function.
>
>> What happens if cgroup_path is "/foo/bar" and _root is "/fo"?
>
> With a mount path of `/bar` this ends up being `/bar/o/bar`. Pretty strange,
> but then again it's a bit of a contrived example as those paths come from
> `/proc` parsing. Anyway, this is code that got added with
> [JDK-8146115](https://bugs.openjdk.java.net/browse/JDK-8146115). It's not
> something I've written and to be honest, I'm not sure this branch is needed,
> but I didn't want to change the existing behaviour with this patch. I have no
> more insight than you in terms of why that approach has been taken.
>
>> Maybe this block should be combined with the new `else` block you're adding?
>
> Maybe, but I'm not sure if it would break something.
>
>> However, the behavior seems opposite between these two blocks of code:
>>
>> The upper block: _root is a prefix of cgroup_path, we take the **tail** of
>> cgroup_path
>>
>> ```
>> TestCase substring_match = {
>> "/sys/fs/cgroup/memory", // mount_path
>> "/user.slice/user-1000.slice", // root_path
>> "/user.slice/user-1000.slice/user@1001.service", // cgroup_path
>> "/sys/fs/cgroup/memory/user@1001.service" // expected_path
>> };
>> ```
>
> Yes. Though, I cannot comment on why that has been chosen. It's been there
> since day one :-/
>
>> The lower block: The beginning of _root is a prefix of cgroup_path, we take
>> the **head** of cgroup_path
>>
>> ```
>> TestCase prefix_matched_cg = {
>> "/sys/fs/cgroup/memory", // mount_path
>> "/user.slice/user-1000.slice/session-50.scope",// root_path
>> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
>> "/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path
>> };
>> ```
>>
>> I find the behavior hard to understand. I think the rules (and reasons)
>> should be added to the comment block above the function.
>
> The reason why I've gone down the path of adding the head of cgroup_path is
> because of this document (in conjunction to what the user was observing on an
> affected system):
> https://www.freedesktop.org/wiki/Software/systemd/ControlGroupInterface/
>
> The user was observing paths as listed in the test:
>
> "/user.slice/user-1000.slice/session-50.scope",// root_path
> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path
>
> This very much looks like systemd managed. Given that and knowing that
> systemd adds processes into `scopes` or `services` and groups them via
> `slices` and also knowing that cgroups are hierarchical (i.e. limits of
> `/foo/bar` also apply to `/foo/bar/baz`), it seems likely that if there are
> any limits, then it'll be on `/user.slice/user-1000.slice` within the mounted
> controller. Unfortunately, I'm not able to reproduce this myself.
I am wondering if the problem is this:
We have systemd running on the host, and a different copy of systemd that runs
inside the container.
- They both set up `/user.slice/user-1000.slice/session-??.scope` within their
own file systems
- For some reason, when you're looking inside the container,
`/proc/self/cgroup` might use a path in the containerized file system whereas
`/proc/self/mountinfo` uses a path in the host file system. These two paths may
look alike but they have absolutely no relation to each other.
I have asked the reporter for more information:
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]
On Wed, 18 May 2022 18:14:52 GMT, Severin Gehwolf wrote:
>> Please review this change to the cgroup v1 subsystem which makes it more
>> resilient on some of the stranger systems. Unfortunately, I wasn't able to
>> re-create a similar system as the reporter. The idea of using the longest
>> substring match for the cgroupv1 file paths was based on the fact that on
>> systemd systems processes run in separate scopes and the maven forked test
>> runner might exhibit this property. For that it makes sense to use the
>> common ancestor path. Nothing changes in the common cases where the
>> `cgroup_path` matches `_root` and when the `_root` is `/` (container the
>> former, host system the latter).
>>
>> In addition, the code paths which are susceptible to throw NPE have been
>> hardened to catch those situations. Should it happen in the future it makes
>> more sense (to me) to not have accurate container detection in favor of
>> continuing to keep running.
>>
>> Finally, with the added unit-tests a bug was uncovered on the "substring"
>> match case of cgroup paths in hotspot. `p` returned from `strstr` can never
>> point to `_root` as it's used as the "needle" to find in "haystack"
>> `cgroup_path` (not the other way round).
>>
>> Testing:
>> - [x] Added unit tests
>> - [x] GHA
>> - [x] Container tests on cgroups v1 Linux. Continue to pass
>
> Severin Gehwolf has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Refactor hotspot gtest
> - Separate into function. Fix comment.
Changes requested by iklam (Reviewer).
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 60:
> 58: strncpy(buf, _mount_point, MAXPATHLEN);
> 59: buf[MAXPATHLEN-1] = '\0';
> 60: _path = os::strdup(buf);
Comment on the old code: why this cannot be simply
_path = os::strdup(_mount_point);
Also, all the strncat calls in this function seem problematic, and should be
converted to stringStream for consistency.
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 63:
> 61: } else {
> 62: char *p = strstr(cgroup_path, _root);
> 63: if (p != NULL && p == cgroup_path) {
What happens if cgroup_path is "/foo/bar" and _root is "/fo"?
Maybe this block should be combined with the new `else` block you're adding?
However, the behavior seems opposite between these two blocks of code:
The upper block: _root is a prefix of cgroup_path, we take the **tail** of
cgroup_path
TestCase substring_match = {
"/sys/fs/cgroup/memory", // mount_path
"/user.slice/user-1000.slice", // root_path
"/user.slice/user-1000.slice/user@1001.service", // cgroup_path
"/sys/fs/cgroup/memory/user@1001.service" // expected_path
};
The lower block: The beginning of _root is a prefix of cgroup_path, we take the
**head** of cgroup_path
TestCase prefix_matched_cg = {
"/sys/fs/cgroup/memory", // mount_path
"/user.slice/user-1000.slice/session-50.scope",// root_path
"/user.slice/user-1000.slice/session-3.scope", // cgroup_path
"/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path
};
I find the behavior hard to understand. I think the rules (and reasons) should
be added to the comment block above the function.
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 86:
> 84: const char* cgroup_p = cgroup_path;
> 85: int last_slash = find_last_slash_pos(root_p, cgroup_p);
> 86: assert(last_slash >= 0, "not an absolute path?");
Are root_p and cgroup_p directly read from the /proc/xxx files. If so, do we
validate the input to make sure they are absolute paths?
It seems like our code cannot handle trailing '/' in the input. If so, we
should clear all trailing '/' from the input string. Then, in functions that
process them, we should assert that they don't end with slash. See my comment
in find_last_slash_pos().
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 102:
> 100: for (int i = 0; *s1 == *s2 && *s1 != 0; i++) {
> 101: if (*s1 == '/') {
> 102: last_matching_slash_pos = i;
I found the behavior a little hard to understand. Is it intentional?
"/cat/cow", "/cat/dog"-> "/cat/"
"/cat/","/cat/dog"-> "/cat/"
"/cat", "/cat/dog"-> "/"
The name `find_last_slash_pos` doesn't properly describe the behavior. I
thought about `find_common_path_prefix`, but that doesn't match the current
behavior (for the 3rd case, the common path prefix is `"/cat"`).
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Thu, 12 May 2022 18:09:40 GMT, Severin Gehwolf wrote: >> Please review this change to the cgroup v1 subsystem which makes it more >> resilient on some of the stranger systems. Unfortunately, I wasn't able to >> re-create a similar system as the reporter. The idea of using the longest >> substring match for the cgroupv1 file paths was based on the fact that on >> systemd systems processes run in separate scopes and the maven forked test >> runner might exhibit this property. For that it makes sense to use the >> common ancestor path. Nothing changes in the common cases where the >> `cgroup_path` matches `_root` and when the `_root` is `/` (container the >> former, host system the latter). >> >> In addition, the code paths which are susceptible to throw NPE have been >> hardened to catch those situations. Should it happen in the future it makes >> more sense (to me) to not have accurate container detection in favor of >> continuing to keep running. >> >> Finally, with the added unit-tests a bug was uncovered on the "substring" >> match case of cgroup paths in hotspot. `p` returned from `strstr` can never >> point to `_root` as it's used as the "needle" to find in "haystack" >> `cgroup_path` (not the other way round). >> >> Testing: >> - [x] Added unit tests >> - [x] GHA >> - [x] Container tests on cgroups v1 Linux. Continue to pass > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Use stringStream to simplify controller path assembly src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 92: > 90: } > 91: ss.print_raw(_root, last_matching_slash_pos); > 92: _path = os::strdup(ss.base()); Do you mean `Find the longest common prefix`? Maybe give an example in the comments? Text parsing in C code is really difficult to understand. - PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Thu, 12 May 2022 18:09:40 GMT, Severin Gehwolf wrote:
>> Please review this change to the cgroup v1 subsystem which makes it more
>> resilient on some of the stranger systems. Unfortunately, I wasn't able to
>> re-create a similar system as the reporter. The idea of using the longest
>> substring match for the cgroupv1 file paths was based on the fact that on
>> systemd systems processes run in separate scopes and the maven forked test
>> runner might exhibit this property. For that it makes sense to use the
>> common ancestor path. Nothing changes in the common cases where the
>> `cgroup_path` matches `_root` and when the `_root` is `/` (container the
>> former, host system the latter).
>>
>> In addition, the code paths which are susceptible to throw NPE have been
>> hardened to catch those situations. Should it happen in the future it makes
>> more sense (to me) to not have accurate container detection in favor of
>> continuing to keep running.
>>
>> Finally, with the added unit-tests a bug was uncovered on the "substring"
>> match case of cgroup paths in hotspot. `p` returned from `strstr` can never
>> point to `_root` as it's used as the "needle" to find in "haystack"
>> `cgroup_path` (not the other way round).
>>
>> Testing:
>> - [x] Added unit tests
>> - [x] GHA
>> - [x] Container tests on cgroups v1 Linux. Continue to pass
>
> Severin Gehwolf has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use stringStream to simplify controller path assembly
test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp line 63:
> 61: ASSERT_STREQ(expected_cg_paths[i], ctrl->subsystem_path());
> 62: }
> 63: }
I found it hard to relate the different paths. Could you create a new struct
like this?
struct TestCase {
char* mount_path;
char* root_paths;
char* cgroup_path;
char* expected_cg_paths;
} = {
{ "/sys/fs/cgroup/memory", // mount
"/", // root,
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems
On Tue, 10 May 2022 12:29:10 GMT, Severin Gehwolf wrote:
> Please review this change to the cgroup v1 subsystem which makes it more
> resilient on some of the stranger systems. Unfortunately, I wasn't able to
> re-create a similar system as the reporter. The idea of using the longest
> substring match for the cgroupv1 file paths was based on the fact that on
> systemd systems processes run in separate scopes and the maven forked test
> runner might exhibit this property. For that it makes sense to use the common
> ancestor path. Nothing changes in the common cases where the `cgroup_path`
> matches `_root` and when the `_root` is `/` (container the former, host
> system the latter).
>
> In addition, the code paths which are susceptible to throw NPE have been
> hardened to catch those situations. Should it happen in the future it makes
> more sense (to me) to not have accurate container detection in favor of
> continuing to keep running.
>
> Finally, with the added unit-tests a bug was uncovered on the "substring"
> match case of cgroup paths in hotspot. `p` returned from `strstr` can never
> point to `_root` as it's used as the "needle" to find in "haystack"
> `cgroup_path` (not the other way round).
>
> Testing:
> - [x] Added unit tests
> - [x] GHA
> - [x] Container tests on cgroups v1 Linux. Continue to pass
I just started to look at the code so just one comment for now.
src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1SubsystemController.java
line 65:
> 63: path = mountPoint + cgroupSubstr;
> 64: }
> 65: } else {
Looks like `path` is still not set if the condition at line 61 `if
(cgroupPath.length() > root.length()) {` is false.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Integrated: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam wrote: > The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never > supported the value `"rw"` since the source code was imported to the openjdk > repo more than 15 years ago. In fact HotSpot throws > `IllegalArgumentException` when such a mode is specified. > > It's unlikely such a mode will be required for future enhancements. Support > for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` > is the only supported mode. > > I also cleaned up related code in the JDK and HotSpot. > > Testing: > - Passed tiers 1 ~ 5 This pull request has now been integrated. Changeset: fcf49f42 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/fcf49f42cef4ac3e50b3b480aecf6fa38cf5be00 Stats: 205 lines in 15 files changed: 3 ins; 153 del; 49 mod 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() Reviewed-by: redestad, alanb - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 20:03:45 GMT, Alan Bateman wrote: >> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never >> supported the value `"rw"` since the source code was imported to the openjdk >> repo more than 15 years ago. In fact HotSpot throws >> `IllegalArgumentException` when such a mode is specified. >> >> It's unlikely such a mode will be required for future enhancements. Support >> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` >> is the only supported mode. >> >> I also cleaned up related code in the JDK and HotSpot. >> >> Testing: >> - Passed tiers 1 ~ 5 > > I skimmed through the changes and I think they look okay. In the distant past > there were tools outside of the JDK that used the jvmstat API directly. It's > possible that VisualVM still does but it would only compile/run if > --add-exports is used to export the sun.jvmstat.* packages. So it might be > that dropping the parameter from a method in RemoteHost is noticed and I > think that is okay because this package is not exported and is not meant to > be used by code outside of the JDK. Thanks to @AlanBateman and @cl4es for the review. - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 20:03:45 GMT, Alan Bateman wrote: > I skimmed through the changes and I think they look okay. In the distant past > there were tools outside of the JDK that used the jvmstat API directly. It's > possible that VisualVM still does but it would only compile/run if > --add-exports is used to export the sun.jvmstat.* packages. So it might be > that dropping the parameter from a method in RemoteHost is noticed and I > think that is okay because this package is not exported and is not meant to > be used by code outside of the JDK. The APIs changed by this PR are: - sun.jvmstat.monitor.remote.RemoteHost::attachVm - sun.jvmstat.monitor.VmIdentifier::getMode - sun.jvmstat.monitor.HostIdentifier::getMode - jdk.internal.perf.Perf::attach I checked the latest VisualVM source code. Some of the above classes are referenced, but none of the affected APIs are called. - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
On Tue, 10 May 2022 19:59:41 GMT, Alan Bateman wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments
>
> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/file/PerfDataBuffer.java
> line 60:
>
>> 58: FileChannel fc = new RandomAccessFile(f, "r").getChannel();
>> 59: ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0L,
>> (int)fc.size());
>> 60: fc.close(); // doesn't need to remain open
>
> I think you can change this to:
>
>
> try (FileChannel fc = FileChannel.open(f.toPath())) {
> ByteBuffer bb = ...
> createPerfDataBuffer(bb, 0);
> }
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
On Tue, 10 May 2022 21:43:44 GMT, Claes Redestad wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> review comments > > src/hotspot/os/windows/perfMemory_windows.cpp line 1781: > >> 1779: // address space. >> 1780: // >> 1781: void PerfMemory::attach(const char* user, int vmid, > > One line? Fixed > src/hotspot/share/prims/perf.cpp line 84: > >> 82: >> 83: // attach to the PerfData memory region for the specified VM >> 84: PerfMemory::attach(user_utf, vmid, > > One line? Fixed > src/hotspot/share/runtime/perfMemory.hpp line 146: > >> 144: // methods for attaching to and detaching from the PerfData >> 145: // memory segment of another JVM process on the same system. >> 146: static void attach(const char* user, int vmid, > > One line? Fixed > src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java line 74: > >> 72: Integer v = lvmid; >> 73: RemoteVm stub = null; >> 74: StringBuilder sb = new StringBuilder(); > > Suggestion: > > String vmidStr = "local://" + lvmid + "@localhost"; Fixed - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never > supported the value `"rw"` since the source code was imported to the openjdk > repo more than 15 years ago. In fact HotSpot throws > `IllegalArgumentException` when such a mode is specified. > > It's unlikely such a mode will be required for future enhancements. Support > for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` > is the only supported mode. > > I also cleaned up related code in the JDK and HotSpot. > > Testing: > - Passed tiers 1 and 2 > - Tiers 3, 4, 5 are in progress Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8622/files - new: https://git.openjdk.java.net/jdk/pull/8622/files/22c22c30..34a01f71 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8622=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8622=00-01 Stats: 14 lines in 5 files changed: 1 ins; 7 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8622.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8622/head:pull/8622 PR: https://git.openjdk.java.net/jdk/pull/8622
RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never supported the value `"rw"` since the source code was imported to the openjdk repo more than 15 years ago. In fact HotSpot throws `IllegalArgumentException` when such a mode is specified. It's unlikely such a mode will be required for future enhancements. Support for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` is the only supported mode. I also cleaned up related code in the JDK and HotSpot. Testing: - Passed tiers 1 and 2 - Tiers 3, 4, 5 are in progress - Commit messages: - 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() Changes: https://git.openjdk.java.net/jdk/pull/8622/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8622=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286441 Stats: 193 lines in 15 files changed: 0 ins; 144 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/8622.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8622/head:pull/8622 PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8284616: Implement workaround for CODETOOLS-7901986 in serviceability/jvmti/RedefineClasses
On Mon, 11 Apr 2022 07:52:34 GMT, Alan Bateman wrote: > > @AlanBateman adding the explicit compile commands to add `--enable-preview` > > is exactly what causes the problem. By compiling that individual file first > > it also compiled some testlib dependencies but puts the classes in a > > different place. When another class is later compiled, the compilation path > > includes that destination and so compilation succeeds, but at runtime the > > path is different and we get the NCDFE. This is the analysis that Alex did > > in a similar issue - see his comment in JBS. > > Adding `--enable-preview` did not intentionally mean to introduce implicit > compilation but it looks like we'll have to re-visit the ordering (as > @alexmenkov suggests) in the loom repo. I believe the underlying cause of this type of failure is https://bugs.openjdk.java.net/browse/CODETOOLS-7902847 "Class directory of a test case should not be used to compile a library" You can easily see the bug by adding this to test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.java in the mainline JDK repo: + * @compile RedefineRunningMethods.java * @run main RedefineClassHelper Clean up your jtreg "work" directory and run this test by itself. Afterwards, look for .class files in the jtreg work dir: ./work/classes/test/lib/RedefineClassHelper.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods$2.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/compiler/InMemoryJavaCompiler$FileManagerWrapper$1.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/compiler/InMemoryJavaCompiler$FileManagerWrapper.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/compiler/InMemoryJavaCompiler$MemoryJavaFileObject.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/compiler/InMemoryJavaCompiler.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/helpers/ClassFileInstaller$Manifest.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/helpers/ClassFileInstaller.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods$1.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods$3.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineClassHelper.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods_B.class You can see that the `@library /test/lib` is split into two parts: /work/classes/test/lib/ - Most of it is in the directory that's used only by the RedefineRunningMethods.java test case - Some of it is in the `work/classes/test/lib/` directory, which is shared by all test cases. Now, create a test case RedefineRunningMethods2.java in the same directory that looks like this: /* * @test * @requires vm.jvmti * @library /test/lib * @modules java.base/jdk.internal.misc * @modules java.compiler * java.instrument * jdk.jartool/sun.tools.jar * @run main RedefineClassHelper */ If you run these two test together, RedefineRunningMethods.java is executed first, leaving the work directory in the same condition as shown above. Then, jtreg runs RedefineRunningMethods2.java, and sees `work/classes/test/lib/RedefineClassHelper.class` is already there. So it executes RedefineClassHelper without rebuilding it, leading to a failure because classes that RedefineClassHelper depends on are not available to this second test. I have a very simple reproducer in the bug above with a detailed log that shows what is happening. While this problem could be worked around, I believe the right fix should be done inside jtreg. - PR: https://git.openjdk.java.net/jdk/pull/8170
Re: RFR: 8284330: jcmd may not be able to find processes in the container [v3]
On Fri, 8 Apr 2022 08:34:24 GMT, Yasumasa Suenaga wrote: >> jcmd uses >> src/jdk.internal.jvmstat/linux/classes/sun/jvmstat/PlatformSupportImpl.java >> to scan temporary directories to find out processes in the container. It >> checks inode to ensure the temp directory is not conflicted. However inode >> maybe same value between the container and others. Thus we should check >> device id for that case. >> >> For example I saw following case on [distroless >> cc-debian11](https://github.com/GoogleContainerTools/distroless/blob/main/cc/README.md) >> container. I started rescue:jdk19 container with sharing PID namespace. >> `/proc/1/root/tmp` is different from `/tmp` on rescue:jdk19, but they are >> same inode value. However we can see the differense in device id. >> >> >> $ podman run -it --rm --entrypoint=sh --pid=container:fa39662f7352 >> rescue:jdk19 >> / # >> / # stat /tmp >> File: /tmp >> Size: 29 Blocks: 0 IO Block: 4096 directory >> Device: efh/239dInode: 135674931 Links: 1 >> Access: (1777/drwxrwxrwt) Uid: (0/root) Gid: (0/root) >> Access: 2022-04-05 08:51:37.0 >> Modify: 2022-04-05 08:51:37.0 >> Change: 2022-04-05 08:51:37.0 >> >> / # stat /proc/1/root/tmp >> File: /proc/1/root/tmp >> Size: 29 Blocks: 0 IO Block: 4096 directory >> Device: e1h/225dInode: 135674931 Links: 1 >> Access: (1777/drwxrwxrwt) Uid: (0/root) Gid: (0/root) >> Access: 2022-04-05 08:51:37.0 >> Modify: 2022-04-05 08:50:42.0 >> Change: 2022-04-05 08:50:42.0 > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Update the comment Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8103
Re: RFR: 8284330: jcmd may not be able to find processes in the container [v2]
On Wed, 6 Apr 2022 12:44:35 GMT, Yasumasa Suenaga wrote:
>> jcmd uses
>> src/jdk.internal.jvmstat/linux/classes/sun/jvmstat/PlatformSupportImpl.java
>> to scan temporary directories to find out processes in the container. It
>> checks inode to ensure the temp directory is not conflicted. However inode
>> maybe same value between the container and others. Thus we should check
>> device id for that case.
>>
>> For example I saw following case on [distroless
>> cc-debian11](https://github.com/GoogleContainerTools/distroless/blob/main/cc/README.md)
>> container. I started rescue:jdk19 container with sharing PID namespace.
>> `/proc/1/root/tmp` is different from `/tmp` on rescue:jdk19, but they are
>> same inode value. However we can see the differense in device id.
>>
>>
>> $ podman run -it --rm --entrypoint=sh --pid=container:fa39662f7352
>> rescue:jdk19
>> / #
>> / # stat /tmp
>> File: /tmp
>> Size: 29 Blocks: 0 IO Block: 4096 directory
>> Device: efh/239dInode: 135674931 Links: 1
>> Access: (1777/drwxrwxrwt) Uid: (0/root) Gid: (0/root)
>> Access: 2022-04-05 08:51:37.0
>> Modify: 2022-04-05 08:51:37.0
>> Change: 2022-04-05 08:51:37.0
>>
>> / # stat /proc/1/root/tmp
>> File: /proc/1/root/tmp
>> Size: 29 Blocks: 0 IO Block: 4096 directory
>> Device: e1h/225dInode: 135674931 Links: 1
>> Access: (1777/drwxrwxrwt) Uid: (0/root) Gid: (0/root)
>> Access: 2022-04-05 08:51:37.0
>> Modify: 2022-04-05 08:50:42.0
>> Change: 2022-04-05 08:50:42.0
>
> Yasumasa Suenaga has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix comments
The code changes look good, but I think the comment should be cleaned up.
src/jdk.internal.jvmstat/linux/classes/sun/jvmstat/PlatformSupportImpl.java
line 117:
> 115: * skip these duplicated directories.
> 116: * Host and container devices could have the same inode value,
> 117: * so we also need to check the device id.
I would suggest rewording the comments from Line 111 to 117 to the following to
be more concise:
* When cgroups is enabled, the directory /proc/{pid}/root/tmp may
* exist even if the given pid is not running inside a container. In
* this case, this directory is usually the same as /tmp and should
* be skipped, or else we would get duplicated hsperfdata files.
* This case can be detected if the inode and device id of
* /proc/{pid}/root/tmp are the same as /tmp.
-
Changes requested by iklam (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8103
Integrated: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM
On Mon, 28 Mar 2022 04:14:32 GMT, Ioi Lam wrote:
> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
> arguments.cpp, which aborts the VM when it fails to allocate a string copy of
> the property value.
>
>
> bool PathString::set_value(const char *value) {
> if (_value != NULL) {
> FreeHeap(_value);
> }
> _value = AllocateHeap(strlen(value)+1, mtArguments );
> // should pass AllocFailStrategy::RETURN_NULL -^
> assert(_value != NULL, "Unable to allocate space for new path value");
>
>
> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
This pull request has now been integrated.
Changeset: 8cdabea0
Author:Ioi Lam
URL:
https://git.openjdk.java.net/jdk/commit/8cdabea0abdc702242a3fb4b0538980ab8f6a9d6
Stats: 41 lines in 4 files changed: 17 ins; 9 del; 15 mod
8207025: JvmtiEnv::SetSystemProperty() does not handle OOM
Reviewed-by: dholmes, sspitsyn
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM
On Mon, 28 Mar 2022 06:19:39 GMT, David Holmes wrote:
>> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
>> arguments.cpp, which aborts the VM when it fails to allocate a string copy
>> of the property value.
>>
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>> FreeHeap(_value);
>> }
>> _value = AllocateHeap(strlen(value)+1, mtArguments );
>> // should pass AllocFailStrategy::RETURN_NULL -^
>> assert(_value != NULL, "Unable to allocate space for new path value");
>>
>>
>> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
>> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
>
> Hi Ioi,
>
> I think the real bug in this code is that
>
> `_value = AllocateHeap(strlen(value)+1, mtArguments);`
>
> should be:
>
> `_value = AllocateHeap(strlen(value)+1, mtArguments,
> AllocFailStrategy::RETURN_NULL);`
>
> as this code is used from `JvmtiEnv::SetSystemProperty` and it seems wrong to
> abort the VM on that path.
Thanks @dholmes-ora and @sspitsyn for the review.
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v4]
> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
> arguments.cpp, which aborts the VM when it fails to allocate a string copy of
> the property value.
>
>
> bool PathString::set_value(const char *value) {
> if (_value != NULL) {
> FreeHeap(_value);
> }
> _value = AllocateHeap(strlen(value)+1, mtArguments );
> // should pass AllocFailStrategy::RETURN_NULL -^
> assert(_value != NULL, "Unable to allocate space for new path value");
>
>
> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
Ioi Lam has updated the pull request with a new target base due to a merge or a
rebase. The incremental webrev excludes the unrelated changes brought in by the
merge/rebase. The pull request contains four additional commits since the last
revision:
- Merge branch 'master' of https://github.com/openjdk/jdk into
8207025-simplify-pathstring-setvalue
- @dholmes-ora comments: simplify the changes
- @dholmes-ora comments: changed implementation to work with
JvmtiEnv::SetSystemProperty
- 8207025: Simplify PathString::set_value() in arguments.cpp
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7981/files
- new: https://git.openjdk.java.net/jdk/pull/7981/files/14a44f63..9fa4cc50
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7981=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7981=02-03
Stats: 1781 lines in 82 files changed: 1348 ins; 78 del; 355 mod
Patch: https://git.openjdk.java.net/jdk/pull/7981.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7981/head:pull/7981
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v3]
On Tue, 29 Mar 2022 04:58:32 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dholmes-ora comments: simplify the changes
>
> src/hotspot/share/runtime/arguments.hpp line 113:
>
>> 111: bool writeable() const { return _writeable; }
>> 112:
>> 113: bool readable() const {
>
> Might be better/simpler to keep is_readable and change to is_writeable(), as
> that avoids changing other files.
There's also `internal()`, which would need to be changed to `is_internal()` as
well for consistency. I chose to change `is_readable()` to `readable()` so I
just needed to change one name.
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v3]
> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
> arguments.cpp, which aborts the VM when it fails to allocate a string copy of
> the property value.
>
>
> bool PathString::set_value(const char *value) {
> if (_value != NULL) {
> FreeHeap(_value);
> }
> _value = AllocateHeap(strlen(value)+1, mtArguments );
> // should pass AllocFailStrategy::RETURN_NULL -^
> assert(_value != NULL, "Unable to allocate space for new path value");
>
>
> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
Ioi Lam has updated the pull request incrementally with one additional commit
since the last revision:
@dholmes-ora comments: simplify the changes
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7981/files
- new: https://git.openjdk.java.net/jdk/pull/7981/files/0397499f..14a44f63
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7981=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7981=01-02
Stats: 29 lines in 4 files changed: 1 ins; 14 del; 14 mod
Patch: https://git.openjdk.java.net/jdk/pull/7981.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7981/head:pull/7981
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM
On Mon, 28 Mar 2022 06:19:39 GMT, David Holmes wrote:
>> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
>> arguments.cpp, which aborts the VM when it fails to allocate a string copy
>> of the property value.
>>
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>> FreeHeap(_value);
>> }
>> _value = AllocateHeap(strlen(value)+1, mtArguments );
>> // should pass AllocFailStrategy::RETURN_NULL -^
>> assert(_value != NULL, "Unable to allocate space for new path value");
>>
>>
>> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
>> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
>
> Hi Ioi,
>
> I think the real bug in this code is that
>
> `_value = AllocateHeap(strlen(value)+1, mtArguments);`
>
> should be:
>
> `_value = AllocateHeap(strlen(value)+1, mtArguments,
> AllocFailStrategy::RETURN_NULL);`
>
> as this code is used from `JvmtiEnv::SetSystemProperty` and it seems wrong to
> abort the VM on that path.
@dholmes-ora To get `JvmtiEnv::SetSystemProperty` working properly, the fix is
more complicated. According to the specification
https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
`JvmtiEnv::SetSystemProperty` should return `JVMTI_ERROR_NOT_AVAILABLE` when
the property is not available or is not writeable, and
JVMTI_ERROR_OUT_OF_MEMORY when OOM (this is one of the "universal errors").
Therefore, I had to change `SystemProperty::set_writeable_value` to return a
tri-state result.
I also added the `JVMTI_ERROR_NULL_POINTER` that was in the spec but wasn't
implemented.
@sspitsyn could you take a look as well?
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8275775: Add jcmd VM.classes to print details of all classes [v10]
On Fri, 4 Mar 2022 09:05:36 GMT, Yi Yang wrote:
>> Add VM.classes to print details of all classes, output looks like:
>>
>> 1. jcmd VM.classes
>>
>> KlassAddr Size State Flags LoaderName ClassName
>> 0x000800c0b400 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$MH/0x000800c0b400
>> 0x000800c0b000 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$DMH/0x000800c0b000
>> 0x000800c0ac00 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$MH/0x000800c0ac00
>> ...
>>
>> 2. jcmd VM.classes verbose
>>
>> KlassAddr Size State Flags LoaderName ClassName
>> 0x000800c0b400 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$MH/0x000800c0b400
>> java.lang.invoke.LambdaForm$MH/0x000800c0b400 {0x000800c0b400}
>> - instance size: 2
>> - klass size: 62
>> - access: final synchronized
>> - state: inited
>> - name: 'java/lang/invoke/LambdaForm$MH+0x000800c0b400'
>> - super: 'java/lang/Object'
>> - sub:
>> - arrays: NULL
>> - methods: Array(0x7f620841f210)
>> - method ordering: Array(0x000800a7e5a8)
>> - default_methods: Array(0x)
>> - local interfaces: Array(0x0008005af748)
>> - trans. interfaces: Array(0x0008005af748)
>> - constants: constant pool [41] {0x7f620841f030} for
>> 'java/lang/invoke/LambdaForm$MH+0x000800c0b400' cache=0x7f620841f380
>> - class loader data: loader data: 0x7f61c804a690 of 'bootstrap' has a
>> class holder
>> - source file: 'LambdaForm$MH'
>> - class annotations: Array(0x)
>> - class type annotations: Array(0x)
>> - field annotations: Array(0x)
>> - field type annotations: Array(0x)
>> - inner classes: Array(0x0008005af6d8)
>> - nest members: Array(0x0008005af6d8)
>> - permitted subclasses: Array(0x0008005af6d8)
>> - java mirror: a 'java/lang/Class'{0x00011f4b3968} =
>> 'java/lang/invoke/LambdaForm$MH+0x000800c0b400'
>> - vtable length 5 (start addr: 0x000800c0b5b8)
>> - itable length 2 (start addr: 0x000800c0b5e0)
>> - static fields (1 words):
>> - static final '_D_0' 'Ljava/lang/invoke/LambdaForm;' @112
>> - non-static fields (0 words):
>> - non-static oop maps:
>> 0x000800c0b000 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$DMH/0x000800c0b000
>> java.lang.invoke.LambdaForm$DMH/0x000800c0b000 {0x000800c0b000}
>> - instance size: 2
>> - klass size: 62
>> - access: final synchronized
>> - state: inited
>> - name: 'java/lang/invoke/LambdaForm$DMH+0x000800c0b000'
>> - super: 'java/lang/Object'
>> - sub:
>> - arrays: NULL
>> - methods: Array(0x7f620841ea68)
>> - method ordering: Array(0x000800a7e5a8)
>> - default_methods: Array(0x)
>> - local interfaces: Array(0x0008005af748)
>> - trans. interfaces: Array(0x0008005af748)
>> - constants: constant pool [49] {0x7f620841e838} for
>> 'java/lang/invoke/LambdaForm$DMH+0x000800c0b000' cache=0x7f620841ebe0
>> - class loader data: loader data: 0x7f61c804a750 of 'bootstrap' has a
>> class holder
>> - source file: 'LambdaForm$DMH'
>> - class annotations: Array(0x)
>> - class type annotations: Array(0x)
>> - field annotations: Array(0x)
>> - field type annotations: Array(0x)
>> - inner classes: Array(0x0008005af6d8)
>> - nest members: Array(0x0008005af6d8)
>> - permitted subclasses: Array(0x0008005af6d8)
>> - java mirror: a 'java/lang/Class'{0x00011f4b0968} =
>> 'java/lang/invoke/LambdaForm$DMH+0x000800c0b000'
>> - vtable length 5 (start addr: 0x000800c0b1b8)
>> - itable length 2 (start addr: 0x000800c0b1e0)
>> - static fields (1 words):
>> - static final '_D_0' 'Ljava/lang/invoke/LambdaForm;' @112
>> - non-static fields (0 words):
>> ...
>
> Yi Yang has updated the pull request incrementally with one additional commit
> since the last revision:
>
> use %4d
LGTM
-
Marked as reviewed by iklam (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7105
Re: RFR: 8275775: Add jcmd VM.classes to print details of all classes [v6]
On Fri, 4 Mar 2022 07:24:51 GMT, Yi Yang wrote: >> You should change it to `%4d`. Otherwise, when the numbers are changed in >> the future (e.g., to 3 or 4 digits) they will be misaligned: >> >> >> KlassAddr Size State FlagsClassName >> 0x000800df8400 62fully_initialized W >> java.lang.invoke.LambdaForm$DMH/0x000800df8400 >> 0x000800df8000 123 fully_initialized W >> java.lang.invoke.LambdaForm$DMH/0x000800df8000 >> 0x000800de4400 4567 fully_initialized W >> java.lang.invoke.LambdaForm$DMH/0x000800de4400 > >> You should change it to `%4d`. Otherwise, when the numbers are changed in >> the future (e.g., to 3 or 4 digits) they will be misaligned: >> >> ``` >> KlassAddr Size State FlagsClassName >> 0x000800df8400 62fully_initialized W >> java.lang.invoke.LambdaForm$DMH/0x000800df8400 >> 0x000800df8000 123 fully_initialized W >> java.lang.invoke.LambdaForm$DMH/0x000800df8000 >> 0x000800de4400 4567 fully_initialized W >> java.lang.invoke.LambdaForm$DMH/0x000800de4400 >> ``` > > This format looks pretty good to me, they are all aligned to left. If you > still think it's more proper to have a format like this: > > KlassAddr Size State FlagsClassName > 0x000800df8400 62fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800df8400 > 0x000800df8000 123 fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800df8000 > 0x000800de4400 4567 fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800de4400 > > Then I'm glad to do so ;) Numbers should be aligned to the right. The following is what I want: 62 123 4567 - PR: https://git.openjdk.java.net/jdk/pull/7105
Re: RFR: 8275775: Add jcmd VM.classes to print details of all classes [v6]
On Fri, 4 Mar 2022 02:47:28 GMT, Yi Yang wrote: >> This issue seem still outstanding. > > Current: > > $./jcmd 83908 VM.classes|head -10 > 83908: > KlassAddr Size State FlagsClassName > 0x000800df8400 62fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800df8400 > 0x000800df8000 62fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800df8000 > 0x000800de4400 62fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800de4400 > 0x000800de4000 62fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800de4000 > 0x000800dc8800 62fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800dc8800 > 0x000800dc8400 62fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800dc8400 > 0x000800dc8000 62fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800dc8000 > 0x000800db9800 62fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800db9800 > > After using "%4d": > > $./jcmd 75481 VM.classes|head > 75481: > KlassAddr Size State FlagsClassName > 0x000800df840062 fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800df8400 > 0x000800df800062 fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800df8000 > 0x000800de440062 fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800de4400 > 0x000800de400062 fully_initialized W > java.lang.invoke.LambdaForm$DMH/0x000800de4000 > > So we do not need to change this. You should change it to `%4d`. Otherwise, when the numbers are changed in the future (e.g., to 3 or 4 digits) they will be misaligned: KlassAddr Size State FlagsClassName 0x000800df8400 62fully_initialized W java.lang.invoke.LambdaForm$DMH/0x000800df8400 0x000800df8000 123 fully_initialized W java.lang.invoke.LambdaForm$DMH/0x000800df8000 0x000800de4400 4567 fully_initialized W java.lang.invoke.LambdaForm$DMH/0x000800de4400 - PR: https://git.openjdk.java.net/jdk/pull/7105
Integrated: 8280543: Update the "java" and "jcmd" tool specification for CDS
On Fri, 28 Jan 2022 01:53:09 GMT, Ioi Lam wrote: > The discussion of CDS in the man pages need to be cleaned up and updated to > match the latest functionalities and intended usage. > > For java.md: > > - Reorganized the flow of the doc: Overview -> How to use -> How to create -> > Restrictions and notes. I think this will be easier to read. > - Added information about `jcmd VM.cds static_dump|dynamic_dump` > - Removed a few sections that are no longer relevant or are uncommon usage > (config files, sharing across two different apps) > - Removed discussion of uncommon error conditions (such as array classes) > - Removed detailed error messages. I think a simple note like "unsupported" > will be good enough for readers of the man page. > - Removed discussion of different version of JDK (these should have been in > release note) > > For jcmd.md: > > - Added some more details about default file name and output directory. > > For ease of reviewing, please see the pre-rendered HTML pages: > > http://cr.openjdk.java.net/~iklam/jdk19/8280543-update-java-and-jcmd-manpage-for-cds2/specs/man/ This pull request has now been integrated. Changeset: 39165613 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/39165613aa0430861e361a33a4925b85ea24fff1 Stats: 509 lines in 2 files changed: 35 ins; 364 del; 110 mod 8280543: Update the "java" and "jcmd" tool specification for CDS Reviewed-by: hseigel, sspitsyn, ccheung - PR: https://git.openjdk.java.net/jdk/pull/7255
Re: RFR: 8280543: Update the "java" and "jcmd" tool specification for CDS [v2]
On Mon, 31 Jan 2022 14:56:27 GMT, Harold Seigel wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed copyright year for jcmd.1 > > LGTM. The copyright in jcmd.1 needs updating. > Thanks, Harold Thanks @hseigel @calvinccheung @sspitsyn for the review. - PR: https://git.openjdk.java.net/jdk/pull/7255
Re: RFR: 8280543: Update the "java" and "jcmd" tool specification for CDS [v2]
> The discussion of CDS in the man pages need to be cleaned up and updated to > match the latest functionalities and intended usage. > > For java.md: > > - Reorganized the flow of the doc: Overview -> How to use -> How to create -> > Restrictions and notes. I think this will be easier to read. > - Added information about `jcmd VM.cds static_dump|dynamic_dump` > - Removed a few sections that are no longer relevant or are uncommon usage > (config files, sharing across two different apps) > - Removed discussion of uncommon error conditions (such as array classes) > - Removed detailed error messages. I think a simple note like "unsupported" > will be good enough for readers of the man page. > - Removed discussion of different version of JDK (these should have been in > release note) > > For jcmd.md: > > - Added some more details about default file name and output directory. > > For ease of reviewing, please see the pre-rendered HTML pages: > > http://cr.openjdk.java.net/~iklam/jdk19/8280543-update-java-and-jcmd-manpage-for-cds2/specs/man/ Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: Fixed copyright year for jcmd.1 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7255/files - new: https://git.openjdk.java.net/jdk/pull/7255/files/edb42155..120e4f86 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7255=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7255=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7255.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7255/head:pull/7255 PR: https://git.openjdk.java.net/jdk/pull/7255
RFR: 8280543: Update the "java" and "jcmd" tool specification for CDS
The discussion of CDS in the man pages need to be cleaned up and updated to match the latest functionalities and intended usage. For java.md: - Reorganized the flow of the doc: Overview -> How to use -> How to create -> Restrictions and notes. I think this will be easier to read. - Added information about `jcmd VM.cds static_dump|dynamic_dump` - Removed a few sections that are no longer relevant or are uncommon usage (config files, sharing across two different apps) - Removed discussion of uncommon error conditions (such as array classes) - Removed detailed error messages. I think a simple note like "unsupported" will be good enough for readers of the man page. - Removed discussion of different version of JDK (these should have been in release note) For jcmd.md: - Added some more details about default file name and output directory. For ease of reviewing, please see the pre-rendered HTML pages: http://cr.openjdk.java.net/~iklam/jdk19/8280543-update-java-and-jcmd-manpage-for-cds2/specs/man/ - Commit messages: - 8280543: Update the "java" and "jcmd" tool specification for CDS Changes: https://git.openjdk.java.net/jdk/pull/7255/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7255=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280543 Stats: 508 lines in 2 files changed: 35 ins; 364 del; 109 mod Patch: https://git.openjdk.java.net/jdk/pull/7255.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7255/head:pull/7255 PR: https://git.openjdk.java.net/jdk/pull/7255
Re: RFR: 8275775: Add jcmd VM.classes to print details of all classes [v6]
On Thu, 27 Jan 2022 09:17:09 GMT, Yi Yang wrote:
>> Add VM.classes to print details of all classes, output looks like:
>>
>> 1. jcmd VM.classes
>>
>> KlassAddr Size State Flags LoaderName ClassName
>> 0x000800c0b400 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$MH/0x000800c0b400
>> 0x000800c0b000 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$DMH/0x000800c0b000
>> 0x000800c0ac00 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$MH/0x000800c0ac00
>> ...
>>
>> 2. jcmd VM.classes verbose
>>
>> KlassAddr Size State Flags LoaderName ClassName
>> 0x000800c0b400 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$MH/0x000800c0b400
>> java.lang.invoke.LambdaForm$MH/0x000800c0b400 {0x000800c0b400}
>> - instance size: 2
>> - klass size: 62
>> - access: final synchronized
>> - state: inited
>> - name: 'java/lang/invoke/LambdaForm$MH+0x000800c0b400'
>> - super: 'java/lang/Object'
>> - sub:
>> - arrays: NULL
>> - methods: Array(0x7f620841f210)
>> - method ordering: Array(0x000800a7e5a8)
>> - default_methods: Array(0x)
>> - local interfaces: Array(0x0008005af748)
>> - trans. interfaces: Array(0x0008005af748)
>> - constants: constant pool [41] {0x7f620841f030} for
>> 'java/lang/invoke/LambdaForm$MH+0x000800c0b400' cache=0x7f620841f380
>> - class loader data: loader data: 0x7f61c804a690 of 'bootstrap' has a
>> class holder
>> - source file: 'LambdaForm$MH'
>> - class annotations: Array(0x)
>> - class type annotations: Array(0x)
>> - field annotations: Array(0x)
>> - field type annotations: Array(0x)
>> - inner classes: Array(0x0008005af6d8)
>> - nest members: Array(0x0008005af6d8)
>> - permitted subclasses: Array(0x0008005af6d8)
>> - java mirror: a 'java/lang/Class'{0x00011f4b3968} =
>> 'java/lang/invoke/LambdaForm$MH+0x000800c0b400'
>> - vtable length 5 (start addr: 0x000800c0b5b8)
>> - itable length 2 (start addr: 0x000800c0b5e0)
>> - static fields (1 words):
>> - static final '_D_0' 'Ljava/lang/invoke/LambdaForm;' @112
>> - non-static fields (0 words):
>> - non-static oop maps:
>> 0x000800c0b000 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$DMH/0x000800c0b000
>> java.lang.invoke.LambdaForm$DMH/0x000800c0b000 {0x000800c0b000}
>> - instance size: 2
>> - klass size: 62
>> - access: final synchronized
>> - state: inited
>> - name: 'java/lang/invoke/LambdaForm$DMH+0x000800c0b000'
>> - super: 'java/lang/Object'
>> - sub:
>> - arrays: NULL
>> - methods: Array(0x7f620841ea68)
>> - method ordering: Array(0x000800a7e5a8)
>> - default_methods: Array(0x)
>> - local interfaces: Array(0x0008005af748)
>> - trans. interfaces: Array(0x0008005af748)
>> - constants: constant pool [49] {0x7f620841e838} for
>> 'java/lang/invoke/LambdaForm$DMH+0x000800c0b000' cache=0x7f620841ebe0
>> - class loader data: loader data: 0x7f61c804a750 of 'bootstrap' has a
>> class holder
>> - source file: 'LambdaForm$DMH'
>> - class annotations: Array(0x)
>> - class type annotations: Array(0x)
>> - field annotations: Array(0x)
>> - field type annotations: Array(0x)
>> - inner classes: Array(0x0008005af6d8)
>> - nest members: Array(0x0008005af6d8)
>> - permitted subclasses: Array(0x0008005af6d8)
>> - java mirror: a 'java/lang/Class'{0x00011f4b0968} =
>> 'java/lang/invoke/LambdaForm$DMH+0x000800c0b000'
>> - vtable length 5 (start addr: 0x000800c0b1b8)
>> - itable length 2 (start addr: 0x000800c0b1e0)
>> - static fields (1 words):
>> - static final '_D_0' 'Ljava/lang/invoke/LambdaForm;' @112
>> - non-static fields (0 words):
>> ...
>
> Yi Yang has updated the pull request incrementally with one additional commit
> since the last revision:
>
> fix
LGTM. One minor nit,
Could you update the PR description to include examples of the final output
format.
src/hotspot/share/oops/instanceKlass.cpp line 2081:
> 2079: _st->print(INTPTR_FORMAT " ", p2i(k));
> 2080: // klass size
> 2081: _st->print("%-4d ", k->size());
Should be `%4d` so that the numbers are aligned correctly.
-
Marked as reviewed by iklam (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7105
Re: RFR: JDK-8280503: Use allStatic.hpp instead of allocation.hpp where possible [v3]
On Mon, 24 Jan 2022 07:41:44 GMT, Thomas Stuefe wrote: >> JDK-8249944 moved AllStatic to its own header. We should use that one >> instead of allocation.hpp where possible to reduce header dependencies. >> >> This patch: >> - replaces includes of allocation.hpp with allstatic.hpp where appropiate >> - fixes up resulting errors since this changes uncovers missing >> dependencies. Mainly, missing includes of debug.hpp, of >> globalDefinitions.hpp, and missing outputStream definitions. >> >> Changes are trivial but onerous. Done partly with a script, partly manually. >> >> Test: >> - Checked the build with gtests on Linux x86, x64, minimal, zero, aarch64, >> for both fastdebug and release. All builds of course without PCH. >> - GHAs > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing header to zNUMA.hpp All builds in our CI passed. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7188
Re: RFR: JDK-8280503: Use allStatic.hpp instead of allocation.hpp where possible [v2]
On Mon, 24 Jan 2022 05:44:50 GMT, Thomas Stuefe wrote: >> JDK-8249944 moved AllStatic to its own header. We should use that one >> instead of allocation.hpp where possible to reduce header dependencies. >> >> This patch: >> - replaces includes of allocation.hpp with allstatic.hpp where appropiate >> - fixes up resulting errors since this changes uncovers missing >> dependencies. Mainly, missing includes of debug.hpp, of >> globalDefinitions.hpp, and missing outputStream definitions. >> >> Changes are trivial but onerous. Done partly with a script, partly manually. >> >> Test: >> - Checked the build with gtests on Linux x86, x64, minimal, zero, aarch64, >> for both fastdebug and release. All builds of course without PCH. >> - GHAs > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > add missing includes for macos, windows > > BTW, I have some scripts for checking how often a header file is included. > > See https://github.com/iklam/tools/tree/main/headers > > Does your tool tell you include chokepoints, maybe its just one central > include pulling in allocation.hpp? > The whoincludes.tcl script can do that. Unfortunately it tells us that many popular header (such as ostream.hpp that was itself included 976 times) include allocations.hpp. src/hotspot$ tclsh whoincludes.tcl allocation.hpp| head -20 scanning997 allocation.hpp 2 found976 ostream.hpp 3 found960 exceptions.hpp 4 found938 atomic.hpp 5 found891 memRegion.hpp 6 found877 iterator.hpp 7 found871 arena.hpp 8 found864 mutex.hpp 9 found860 growableArray.hpp 10 found855 mutexLocker.hpp 11 found855 autoRestore.hpp 12 found848 padded.hpp 13 found841 linkedlist.hpp 14 found835 jfrAllocation.hpp 15 found832 resourceHash.hpp 16 found829 gcUtil.hpp 17 found825 threadHeapSampler.hpp 18 found825 thread.hpp 19 found825 filterQueue.hpp 20 found679 symbol.hpp - PR: https://git.openjdk.java.net/jdk/pull/7188
Re: RFR: JDK-8280503: Use allStatic.hpp instead of allocation.hpp where possible
On Mon, 24 Jan 2022 04:18:48 GMT, Ioi Lam wrote: > I am running a mach5 job for tier1 + builds-tier5. That should cover most of > the builds done by the Oracle CI. I'll post the results when they are ready. Unfortunately I am seeing failures on macos and windows: macos: src/hotspot/os/bsd/gc/z/zNUMA_bsd.cpp:25: src/hotspot/share/gc/z/zNUMA.hpp:39:10: error: unknown type name 'uint32_t' windows: src\hotspot\os\windows\threadLocalStorage_windows.cpp(34): error C3861: 'assert': identifier not found - PR: https://git.openjdk.java.net/jdk/pull/7188
Re: RFR: JDK-8280503: Use allStatic.hpp instead of allocation.hpp where possible
On Sat, 22 Jan 2022 13:33:24 GMT, Thomas Stuefe wrote: > JDK-8249944 moved AllStatic to its own header. We should use that one instead > of allocation.hpp where possible to reduce header dependencies. > > This patch: > - replaces includes of allocation.hpp with allstatic.hpp where appropiate > - fixes up resulting errors since this changes uncovers missing dependencies. > Mainly, missing includes of debug.hpp, of globalDefinitions.hpp, and missing > outputStream definitions. > > Changes are trivial but onerous. Done partly with a script, partly manually. > > Test: > - Checked the build with gtests on Linux x86, x64, minimal, zero, aarch64, > for both fastdebug and release. All builds of course without PCH. > - GHAs BTW, I have some scripts for checking how often a header file is included. See https://github.com/iklam/tools/tree/main/headers count_hotspot_headers.tcl shows that allocation.hpp was included by 1006 .o files before this fix, and 996 files afterwards, so not a whole lot of reduction. That's because we have over 300 headers that include allocatons.hpp :-) - PR: https://git.openjdk.java.net/jdk/pull/7188
Re: RFR: JDK-8280503: Use allStatic.hpp instead of allocation.hpp where possible
On Sat, 22 Jan 2022 13:33:24 GMT, Thomas Stuefe wrote: > JDK-8249944 moved AllStatic to its own header. We should use that one instead > of allocation.hpp where possible to reduce header dependencies. > > This patch: > - replaces includes of allocation.hpp with allstatic.hpp where appropiate > - fixes up resulting errors since this changes uncovers missing dependencies. > Mainly, missing includes of debug.hpp, of globalDefinitions.hpp, and missing > outputStream definitions. > > Changes are trivial but onerous. Done partly with a script, partly manually. > > Test: > - Checked the build with gtests on Linux x86, x64, minimal, zero, aarch64, > for both fastdebug and release. All builds of course without PCH. > - GHAs Looks good to me. I've validated with these builds locally on my machine: linux-x64-debug linux-aarch64-open-debug linux-arm32 linux-ppc64le-debug linux-s390x-debug linux-aarch64-debug linux-arm32-open-debug linux-aarch64-lic I am running a mach5 job for tier1 + builds-tier5. That should cover most of the builds done by the Oracle CI. I'll post the results when they are ready. - PR: https://git.openjdk.java.net/jdk/pull/7188
Re: RFR: 8275775: Add jcmd VM.classes to print details of all classes [v5]
On Thu, 20 Jan 2022 09:47:31 GMT, Yi Yang wrote:
>> Add VM.classes to print details of all classes, output looks like:
>>
>> 1. jcmd VM.classes
>>
>> KlassAddr Size State Flags LoaderName ClassName
>> 0x000800c0b400 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$MH/0x000800c0b400
>> 0x000800c0b000 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$DMH/0x000800c0b000
>> 0x000800c0ac00 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$MH/0x000800c0ac00
>> ...
>>
>> 2. jcmd VM.classes verbose
>>
>> KlassAddr Size State Flags LoaderName ClassName
>> 0x000800c0b400 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$MH/0x000800c0b400
>> java.lang.invoke.LambdaForm$MH/0x000800c0b400 {0x000800c0b400}
>> - instance size: 2
>> - klass size: 62
>> - access: final synchronized
>> - state: inited
>> - name: 'java/lang/invoke/LambdaForm$MH+0x000800c0b400'
>> - super: 'java/lang/Object'
>> - sub:
>> - arrays: NULL
>> - methods: Array(0x7f620841f210)
>> - method ordering: Array(0x000800a7e5a8)
>> - default_methods: Array(0x)
>> - local interfaces: Array(0x0008005af748)
>> - trans. interfaces: Array(0x0008005af748)
>> - constants: constant pool [41] {0x7f620841f030} for
>> 'java/lang/invoke/LambdaForm$MH+0x000800c0b400' cache=0x7f620841f380
>> - class loader data: loader data: 0x7f61c804a690 of 'bootstrap' has a
>> class holder
>> - source file: 'LambdaForm$MH'
>> - class annotations: Array(0x)
>> - class type annotations: Array(0x)
>> - field annotations: Array(0x)
>> - field type annotations: Array(0x)
>> - inner classes: Array(0x0008005af6d8)
>> - nest members: Array(0x0008005af6d8)
>> - permitted subclasses: Array(0x0008005af6d8)
>> - java mirror: a 'java/lang/Class'{0x00011f4b3968} =
>> 'java/lang/invoke/LambdaForm$MH+0x000800c0b400'
>> - vtable length 5 (start addr: 0x000800c0b5b8)
>> - itable length 2 (start addr: 0x000800c0b5e0)
>> - static fields (1 words):
>> - static final '_D_0' 'Ljava/lang/invoke/LambdaForm;' @112
>> - non-static fields (0 words):
>> - non-static oop maps:
>> 0x000800c0b000 62 inited W bootstrap
>> java.lang.invoke.LambdaForm$DMH/0x000800c0b000
>> java.lang.invoke.LambdaForm$DMH/0x000800c0b000 {0x000800c0b000}
>> - instance size: 2
>> - klass size: 62
>> - access: final synchronized
>> - state: inited
>> - name: 'java/lang/invoke/LambdaForm$DMH+0x000800c0b000'
>> - super: 'java/lang/Object'
>> - sub:
>> - arrays: NULL
>> - methods: Array(0x7f620841ea68)
>> - method ordering: Array(0x000800a7e5a8)
>> - default_methods: Array(0x)
>> - local interfaces: Array(0x0008005af748)
>> - trans. interfaces: Array(0x0008005af748)
>> - constants: constant pool [49] {0x7f620841e838} for
>> 'java/lang/invoke/LambdaForm$DMH+0x000800c0b000' cache=0x7f620841ebe0
>> - class loader data: loader data: 0x7f61c804a750 of 'bootstrap' has a
>> class holder
>> - source file: 'LambdaForm$DMH'
>> - class annotations: Array(0x)
>> - class type annotations: Array(0x)
>> - field annotations: Array(0x)
>> - field type annotations: Array(0x)
>> - inner classes: Array(0x0008005af6d8)
>> - nest members: Array(0x0008005af6d8)
>> - permitted subclasses: Array(0x0008005af6d8)
>> - java mirror: a 'java/lang/Class'{0x00011f4b0968} =
>> 'java/lang/invoke/LambdaForm$DMH+0x000800c0b000'
>> - vtable length 5 (start addr: 0x000800c0b1b8)
>> - itable length 2 (start addr: 0x000800c0b1e0)
>> - static fields (1 words):
>> - static final '_D_0' 'Ljava/lang/invoke/LambdaForm;' @112
>> - non-static fields (0 words):
>> ...
>
> Yi Yang has updated the pull request incrementally with one additional commit
> since the last revision:
>
> fix test
This looks generally OK to me. Some minor suggestions.
src/hotspot/share/oops/instanceKlass.cpp line 2106:
> 2104: // classloader name
> 2105: ClassLoaderData* cld = k->class_loader_data();
> 2106: _st->print("%-12s ", cld->loader_name());
For custom class loaders, this will likely print a long class name that will
over the 12 character limit, making the output somewhat hard to read.
const char* ClassLoaderData::loader_name() const {
if (_class_loader_klass == NULL) {
return BOOTSTRAP_LOADER_NAME;
} else if (_name != NULL) {
return _name->as_C_string();
} else {
return _class_loader_klass->external_name();
}
}
Also, for custom loaders, printing out just the name of the loader class is not
sufficient, as multiple loader instances may have the same type.
Maybe we should just remove line 2106? If the user wants to know the class
loader, they can use the "-verbose" option of this jcmd.
Re: RFR: 8275775: Add jcmd VM.classes to print details of all classes [v2]
On Wed, 19 Jan 2022 08:39:34 GMT, Xin Liu wrote: > > How about this: > > ``` > > jcmd VM.classes -verbose classname classname ... > > ``` > > > > -verbose is optional > > more than one classnames can be specified. > > if no classnames are specified, all classes are printed > > If the class name here means the "fully-qualified" class name, I guess it's > not practical to input multiple classnames like > "java.lang.invoke.LambdaForm$MH/0x000800c0b400" in cmdline. > > The main cost of VM_PrintClasses should be the traversal of all classes. I > feel a filter won't save much runtime time. We can leave it to the external > awk scripts. What do you think? That sounds fair. I think for filtering it's best left to external tools. I think we should use `-verbose` as the optional argument, to be consistent with other jcmds such as ` VM.symboltable` - PR: https://git.openjdk.java.net/jdk/pull/7105
Re: RFR: 8275775: Add jcmd VM.classes to print details of all classes [v2]
On Wed, 19 Jan 2022 02:50:16 GMT, Chris Plummer wrote: > > > It seems it would be useful to support the verbose output with just a > > > single class that is specified, although that would suggest that the dcmd > > > name should then be something other than `VM.classes`. > > > > > > This is a good idea, but `jcmd VM.classes verbose=XX` looks strange, `jcmd > > VM.class XX` is also not much proper, because we desire to print all > > classes in default(`jcmd VM.class`). an alternative is to use `jcmd > > VM.classes verbose | grep XX` currently. > > I was thinking the syntax would look like: `jcmd VM.classes [verbose > [classname]]` > > Your grep solution doesn't work because each class has multiple lines of > output. How about this: jcmd VM.classes -verbose classname classname ... -verbose is optional more than one classnames can be specified. if no classnames are specified, all classes are printed - PR: https://git.openjdk.java.net/jdk/pull/7105
Re: RFR: 8277481: Obsolete seldom used CDS flags
On Fri, 10 Dec 2021 15:01:29 GMT, Harold Seigel wrote: > Please review this change to obsolete deprecated CDS options UseSharedSpaces, > RequireSharedSpaces, DynamicDumpSharedSpaces, and DumpSharedSpaces. The > change was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows > and Mach5 tiers 3-5 on Linux x64 and Windows x64. > > The use of UseSharedSpaces in ps_core_common.c was tested on Mac OS x64 by > temporarily removing serviceability/sa/ClhsdbPmap.java#core from the problem > list. > > Thanks! Harold Looks good to me. Thanks for fixing this! - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6800
Re: RFR: JDK-8276983: Small fixes to DumpAllocStat::print_stats
On Thu, 11 Nov 2021 06:15:32 GMT, Thomas Stuefe wrote: > When looking at CDS code in the context of Lilliput, I had to spend some time > in DumpAllocStat::print(). I noticed two small things which can be fixed > independently: > > - the divide-by-zero check at lines 45ff is not needed, since `percent_of` > does this already. It also can cause the asserts at the end of the function > to fire wrongly. > > - About those asserts: it makes sense to flush the debug message before scope > end, otherwise, we won't see the debug message if the asserts fire. If they > fire, the debug message would be helpful. > > I also improved the assert message somewhat. I noticed that all sizes in this > method are `int`, but left it that way because changing it would have too > many ripple effects. I guess we won't get CDS archives beyond int_max any > time soon. > > Thanks, Thomas LGTM. Good clean up. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6347
Re: RFR: 8275259: Add support for Java level DCmd
On 11/9/21 2:07 PM, Chris Plummer wrote: On 10/14/21 12:38 AM, David Holmes wrote: On 14/10/2021 5:14 pm, Denghui Dong wrote: On Thu, 14 Oct 2021 06:43:20 GMT, David Holmes wrote: I'm not sure exactly where this API would need to go. IIUC jcmd doesn't exist at the Java level it is just a tool, so introducing an API to interact with it seems problematic to me. IMO, `jcmd` is a client of `dcmd`, the user also can use JMX to execute `dcmd`, this means there already are Java APIs interact with `dcmd`. Ah right - so you're really looking at extending the capabilities of the DiagnosticCommandMBean to add a way to register a Java diagnostic command. David Just reviewing some previous emails and noticed this one since I recently looked into the JMX support. JMX supports a rather generic concept of executing diagnostics commands (there is no actual reference to "dcmds"). See com.sun.management.DiagnosticCommandMBean and its superinterface javax.management.DynamicMBean: https://docs.oracle.com/en/java/javase/15/docs/api/jdk.management/com/sun/management/DiagnosticCommandMBean.html https://docs.oracle.com/en/java/javase/15/docs/api/java.management/javax/management/DynamicMBean.html DynamicMBean provides the invoke() interface, which when used on a DiagnosticCommandMBean will invoke the specified diagnostic command. It says nothing about how diagnostic commands are implemented or registered with the jvm. That I assume would be JVM implementation dependent. Ah right - so you're really looking at extending the capabilities of the DiagnosticCommandMBean to add a way to register a Java diagnostic command. Yes to "add a way to register a Java diagnostic command", but the new API will not involve DiagnosticCommandMBean in any way. The new API will interface with the existing hotspot native support for dcmds to allow dcmds to be implemented by and registered from java. In any case, I think having an API that allows you to invoke a diagnostic command from java is very different in scope from an API that allows you to implement one in java, so I'm not so sure this JMX precedence serves us much purpose here. Is there anything in the proposal that cannot be done by JMX? If the app wants to provide a hook to expose some of its internals to be monitored/controlled by an external tool, maybe it can just implement a new MBean? Granted, it's bit more tedious to connect to JMX than using "jcmd". However, much of that is for security. If we allow an app to implement new jcmd handlers using Java code, we have to think about the whole security model again. Are we reinventing the wheel here? Thanks - Ioi
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
On Sat, 6 Nov 2021 12:16:10 GMT, Serguei Spitsyn wrote: > Hi Ioi, > Sorry for being late here. > Just to complete this... > Thank you for your answers! I'm okay with them. Thanks Serguei! - PR: https://git.openjdk.java.net/jdk/pull/5923
Integrated: 8275185: Remove dead code and clean up jvmstat LocalVmManager
On Wed, 13 Oct 2021 04:17:25 GMT, Ioi Lam wrote: > LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs > owned by a specific user. No one uses these APIs, and they don't work anyway. > > The current code is very confusing to look at. Since we're likely to change > code in this area for further container support, it's better to clean up the > code now. > > - Remove all APIs that take a user name > - Also removed PerfDataFile.getFile() methods that are unused > - Cleaned up the code that looks up the hsperfdata_xxx files > - Fix comments to explain what's happening > - Avoid using Matcher.reset which is not thread-safe > - Renamed confusing variables such as `userFilter` to make the code more > readable > - LocalVmManager.activeVms() probably doesn't need to be synchronized, but > I kept it anyway to avoid unnecessary risks. > > Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers > and have extensive use of the management tools). This pull request has now been integrated. Changeset: 8e17ce00 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/8e17ce00316a765bbedefc34dc5898ba4f3f2144 Stats: 296 lines in 2 files changed: 9 ins; 255 del; 32 mod 8275185: Remove dead code and clean up jvmstat LocalVmManager Reviewed-by: cjplummer, redestad, kevinw - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
On Thu, 4 Nov 2021 03:44:43 GMT, Chris Plummer wrote: >> Ioi Lam has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains three additional commits >> since the last revision: >> >> - Merge branch 'master' into >> 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code >> - @kevinjwalls and @plummercj review - (1) restore >> PerfDataFile.userDirNamePattern, etc. (2) Fixed comments >> - 8275185: Remove dead code and clean up jvmstat LocalVmManager > > Marked as reviewed by cjplummer (Reviewer). Thanks @plummercj @kevinjwalls @cl4es for the review. I filed https://bugs.openjdk.java.net/browse/JDK-8276687 to remove the JDK 1.4.1 support. Passed Oracle CI tiers1-4 - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
On Tue, 19 Oct 2021 22:02:58 GMT, Chris Plummer wrote:
>> Ioi Lam has updated the pull request with a new target base due to a merge
>> or a rebase. The incremental webrev excludes the unrelated changes brought
>> in by the merge/rebase. The pull request contains three additional commits
>> since the last revision:
>>
>> - Merge branch 'master' into
>> 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code
>> - @kevinjwalls and @plummercj review - (1) restore
>> PerfDataFile.userDirNamePattern, etc. (2) Fixed comments
>> - 8275185: Remove dead code and clean up jvmstat LocalVmManager
>
> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/LocalVmManager.java
> line 75:
>
>> 73: // 1.4.1 (or earlier?): the files are stored directly under
>> {tmpdir}/ with
>> 74: // the following pattern.
>> 75: Pattern oldtmpFilePattern =
>> Pattern.compile("^hsperfdata_[0-9]+(_[1-2]+)?$");
>
> So this pattern optionally has `_` followed by a sequence of 1's and 2's at
> the end? Seems odd.
I restored this line to use PerfDataFile.tmpFileNamePattern, as before my
changes. Yes, that's an odd way of naming a file.
> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/LocalVmManager.java
> line 105:
>
>> 103:
>> 104:
>> 105: // 1.4.2 and later: Look for the files
>> {tmpdir}/hsperfdata_{any_user_name}/[0-0]+
>
> should be `[0-9]+`
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager
On Tue, 26 Oct 2021 07:48:05 GMT, Kevin Walls wrote: > Nice cleanup - although was it not better for PerfDataFile.java to "own" the > definitions of what a perfdata filename looks like? That might be what Chris > was hinting at. There isn't really that _much_ flipping between two files. 8-) Hi Kevin, thanks for the review. I've moved the filename patterns back to PerfDataFile.java as you suggested. - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager
On Wed, 20 Oct 2021 02:23:25 GMT, Serguei Spitsyn wrote: > Hi Ioi, It looks good to me except the line 105 commented by Chris. I wonder > how should this be tested to make sure there are no regressions. Do we really > care about pre 1.4.2 formats? If so, what is the way to test it? We have a > little lack of knowledge in this area. I guess, we need the performance team > to get involved into this review. Thanks, Serguei I am not sure about 1.4.2 formats. I guess we don't really care, but we should probably remove it in a separate RFE. I am trying to keep this PR from having any functional changes, and will just remove dead code. For testing, I am running tiers 1-4. Will that be enough? I'll ping the performance team. Thanks - Ioi - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
> LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs > owned by a specific user. No one uses these APIs, and they don't work anyway. > > The current code is very confusing to look at. Since we're likely to change > code in this area for further container support, it's better to clean up the > code now. > > - Remove all APIs that take a user name > - Also removed PerfDataFile.getFile() methods that are unused > - Cleaned up the code that looks up the hsperfdata_xxx files > - Fix comments to explain what's happening > - Avoid using Matcher.reset which is not thread-safe > - Renamed confusing variables such as `userFilter` to make the code more > readable > - LocalVmManager.activeVms() probably doesn't need to be synchronized, but > I kept it anyway to avoid unnecessary risks. > > Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers > and have extensive use of the management tools). Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' into 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code - @kevinjwalls and @plummercj review - (1) restore PerfDataFile.userDirNamePattern, etc. (2) Fixed comments - 8275185: Remove dead code and clean up jvmstat LocalVmManager - Changes: - all: https://git.openjdk.java.net/jdk/pull/5923/files - new: https://git.openjdk.java.net/jdk/pull/5923/files/c2f3937b..bbeb7c16 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5923=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5923=00-01 Stats: 43203 lines in 1054 files changed: 33107 ins; 6126 del; 3970 mod Patch: https://git.openjdk.java.net/jdk/pull/5923.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5923/head:pull/5923 PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8276208: vmTestbase/nsk/jdb/repeat/repeat001/repeat001.java fails with "AssertionError: Unexpected output" [v2]
On Tue, 2 Nov 2021 01:07:30 GMT, Jakob Cornell wrote: >> This will fix a few issues with the tests added in #5290: >> >> - [x] intermittent failures >> - [x] tests should use `failure` method to report problems rather than >> throwing `AssertionError` > > Jakob Cornell has updated the pull request incrementally with one additional > commit since the last revision: > > Fix incorrect use of `receiveReplyFor' causing test failures LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6182
Re: RFR: 8271356: Modify jdb to treat an empty command as a repeat of the previous command [v8]
On Mon, 25 Oct 2021 22:21:54 GMT, Jakob Cornell wrote: >> This has been under discussion on and off for the past month or so on >> serviceability-dev, and I think a CSR request is required, so this may be a >> work in progress. >> >> Notes on the patch: >> >> - The `list` command previously marked a line in each listing with `=>`. In >> a bare `list` this is the next line up for execution. Previously when >> requesting a specific location (e.g. `list 5`) the requested line would be >> marked. With the patch applied, `list` will only ever mark the next line up >> for execution. This is consistent with the behavior of GDB and PDB (at >> least). >> - `EOF` is printed when the repeat setting is on and a bare `list` command >> follows a listing containing the last source line. This feature is from >> PDB; it's a somewhat softer message than the one for an explicit `list` >> request that's out of range. >> - I don't speak Chinese or Japanese, so I've omitted localizations for the >> new messages in those locales. However, I updated the help text in both to >> include the new commands, with the descriptions left empty for now. > > Jakob Cornell has updated the pull request incrementally with one additional > commit since the last revision: > > Restore update of copyright messages in resource files LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5290
Re: RFR: 8271356: Modify jdb to treat an empty command as a repeat of the previous command [v5]
On Tue, 19 Oct 2021 21:26:11 GMT, Chris Plummer wrote: > > Hi Jacob, this is not your fault, but the "zz help text" in the Chinese and > > Japanese versions are in a single huge line. This makes it impossible to > > see what you have changed in the GitHub diffs, and impossible to tell > > whether you made any typos in the process. > > I'd suggest you just undo the change. As mentioned earlier, localization is > not your responsibility, and this lack of localization won't cause jdb to not > run properly. It will just be missing some help text. Sounds good to me, too. Just remember to file a bug to update the localized text. - PR: https://git.openjdk.java.net/jdk/pull/5290
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager
On Tue, 19 Oct 2021 21:17:57 GMT, Chris Plummer wrote: >> LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs >> owned by a specific user. No one uses these APIs, and they don't work anyway. >> >> The current code is very confusing to look at. Since we're likely to change >> code in this area for further container support, it's better to clean up the >> code now. >> >> - Remove all APIs that take a user name >> - Also removed PerfDataFile.getFile() methods that are unused >> - Cleaned up the code that looks up the hsperfdata_xxx files >> - Fix comments to explain what's happening >> - Avoid using Matcher.reset which is not thread-safe >> - Renamed confusing variables such as `userFilter` to make the code more >> readable >> - LocalVmManager.activeVms() probably doesn't need to be synchronized, but >> I kept it anyway to avoid unnecessary risks. >> >> Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers >> and have extensive use of the management tools). > > src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/PerfDataFile.java > line 80: > >> 78: "^hsperfdata_[0-9]+(_[1-2]+)?$"; >> 79: >> 80: > > I don't understand why you thought it best to remove these and instead use > hard coded references to these partterns. I have two goals - these aren't used anywhere else, so it's better to at least move them from this file to LocalVmManager.java, where they are actually used. So you don't need to flip between two files. - the behavior is easier to understand if the string literals, comments, and the code that uses them are together. Note that the original code has plenty of comment that are rather useless if you want to know how the files are searched. - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8271356: Modify jdb to treat an empty command as a repeat of the previous command [v5]
On Thu, 23 Sep 2021 15:24:47 GMT, Jakob Cornell wrote: >> This has been under discussion on and off for the past month or so on >> serviceability-dev, and I think a CSR request is required, so this may be a >> work in progress. >> >> Notes on the patch: >> >> - The `list` command previously marked a line in each listing with `=>`. In >> a bare `list` this is the next line up for execution. Previously when >> requesting a specific location (e.g. `list 5`) the requested line would be >> marked. With the patch applied, `list` will only ever mark the next line up >> for execution. This is consistent with the behavior of GDB and PDB (at >> least). >> - `EOF` is printed when the repeat setting is on and a bare `list` command >> follows a listing containing the last source line. This feature is from >> PDB; it's a somewhat softer message than the one for an explicit `list` >> request that's out of range. >> - I don't speak Chinese or Japanese, so I've omitted localizations for the >> new messages in those locales. However, I updated the help text in both to >> include the new commands, with the descriptions left empty for now. > > Jakob Cornell has updated the pull request incrementally with one additional > commit since the last revision: > > 8271356: Invoke auto-advance on empty input after targeted list command Hi Jacob, this is not your fault, but the "zz help text" in the Chinese and Japanese versions are in a single huge line. This makes it impossible to see what you have changed in the GitHub diffs, and impossible to tell whether you made any typos in the process. I would recommend making a prerequisite PR first: - Break the huge lines in those two files in the same way as TTYResources.java. Verify that TTYResources_ja.class, etc, are identical to the previous version. Integrate into openjdk. - Revert the changes of those two files in the current PR - Merge with the prerequisite PR - Add the new lines into those two files Thanks - Ioi - Changes requested by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5290
RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager
LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs owned by a specific user. No one uses these APIs, and they don't work anyway. The current code is very confusing to look at. Since we're likely to change code in this area for further container support, it's better to clean up the code now. - Remove all APIs that take a user name - Also removed PerfDataFile.getFile() methods that are unused - Cleaned up the code that looks up the hsperfdata_xxx files - Fix comments to explain what's happening - Avoid using Matcher.reset which is not thread-safe - Renamed confusing variables such as `userFilter` to make the code more readable - LocalVmManager.activeVms() probably doesn't need to be synchronized, but I kept it anyway to avoid unnecessary risks. Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers and have extensive use of the management tools). - Commit messages: - 8275185: Remove dead code and clean up jvmstat LocalVmManager Changes: https://git.openjdk.java.net/jdk/pull/5923/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5923=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275185 Stats: 319 lines in 2 files changed: 9 ins; 278 del; 32 mod Patch: https://git.openjdk.java.net/jdk/pull/5923.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5923/head:pull/5923 PR: https://git.openjdk.java.net/jdk/pull/5923
Re: Extend jcmd to java application level
On 10/7/21 6:25 PM, David Holmes wrote:
Hi Denghui,
On 7/10/2021 11:58 pm, Denghui Dong wrote:
Hi team,
The `jcmd` command can be used to call some built-in diagnostic commands in vm.
Can we consider extending it to the java layer like perf data, so that Java developers can
customize their diagnostic commands and then call them through `jcmd`?
One application scenario I can think of for this extension is that some statistical information
may be collected in a java application. Triggering the output of this statistical information through
the `jcmd` command seems to me relative to other mechanisms that trigger output (such as through
an HTTP service, or periodic Printing) is more convenient.
Any input is appreciated.
If the intent is that you could issue a jcmd:
jcmd MyClass.foo
to have it run/use a Java thread to execute arbitrary code then I
think a lot of careful consideration would need to be given to making
this facility safe and secure. I can easily imagine that the thread
used, and the timing, could cause failures. Executing arbitrary code
may be far too general, so it might mean we need a new "service"
interface defined that the target class has to implement.
It might well be useful but will need some deep thought IMO.
If I understood correctly, the app would need to call an API like:
JcmdProvider.register("mycmd1", new JcmdHandler() {
public void handleCommand(String args[], PrintStream out) {
out.print("my response is ");
... }
});
and then the user can issue:
jcmd mycmd1 args .
which will reach the handleCommand() method provided by the app.
Thanks
- Ioi
Cheers,
David
Thanks,
Denghui Dong
Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v8]
On Wed, 6 Oct 2021 21:53:30 GMT, Yumin Qi wrote: >> Please review, >> Refactor fundamental CDS FileMapHeader code for reliable reading of basic >> info from shared archive. >> With the change, it makes it possible to read an archive generated by >> different version of hotspot. Also it is possible to automatically generate >> a CDS archive If the archive supplied is not readable or fails to pass the >> check. >> >> Tests: tier1-4 >>jtreg on sa. >> >> Thanks >> Yumin > > Yumin Qi has updated the pull request incrementally with one additional > commit since the last revision: > > Add offset check for _generic_header in populate_header LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5768
Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v5]
On Wed, 6 Oct 2021 16:44:25 GMT, Yumin Qi wrote: >> src/hotspot/share/cds/filemap.cpp line 1205: >> >>> 1203: } >>> 1204: >>> 1205: _header = (FileMapHeader*)os::malloc(gen_header->_header_size, >>> mtInternal); >> >> There's no need to allocate and read the header again. It's already in >> gen_header. This should be enough: >> >> >> _header = (FileMapHeader*)gen_header; > > see above reply. We need read the full _header_size for _header. Also note > that helper class will delete gen_header when out of scope. I see. I think your current code is fine. Note that the current code writes the header as a FileMapHeader, but it reads it as both a FileMapHeader and a CDSFileMapHeaderBase. So it has the basic assumption `FileMapHeader::_generic_header` is at offset 0 of FileMapHeader. Therefore, in FileHeaderHelper::initialize, we should add an assert: static_assert(offsetof(FileMapHeader, _generic_header) == 0, "must be"); - PR: https://git.openjdk.java.net/jdk/pull/5768
Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v6]
On Wed, 6 Oct 2021 17:01:31 GMT, Yumin Qi wrote:
>> Please review,
>> Refactor fundamental CDS FileMapHeader code for reliable reading of basic
>> info from shared archive.
>> With the change, it makes it possible to read an archive generated by
>> different version of hotspot. Also it is possible to automatically generate
>> a CDS archive If the archive supplied is not readable or fails to pass the
>> check.
>>
>> Tests: tier1-4
>>jtreg on sa.
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed comment for magic check, fixed _file_offset to _header_size
src/hotspot/share/cds/filemap.cpp line 1045:
> 1043: class FileHeaderHelper {
> 1044: int _fd;
> 1045: GenericCDSFileMapHeader* _header;
This should be changed to `GenericCDSFileMapHeader _header;`. That way, you
don't need to malloc and free it.
-
PR: https://git.openjdk.java.net/jdk/pull/5768
Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v5]
On Tue, 5 Oct 2021 22:32:30 GMT, Yumin Qi wrote:
>> Please review,
>> Refactor fundamental CDS FileMapHeader code for reliable reading of basic
>> info from shared archive.
>> With the change, it makes it possible to read an archive generated by
>> different version of hotspot. Also it is possible to automatically generate
>> a CDS archive If the archive supplied is not readable or fails to pass the
>> check.
>>
>> Tests: tier1-4
>>jtreg on sa.
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added a helper class to facilitate checking archive
Looks good overall. Just a few nits.
src/hotspot/share/cds/filemap.cpp line 278:
> 276: }
> 277:
> 278: // Do not check _magic, it's not been set yet.
I think you can get rid of this comment by moving line 228 .. 231 after line
233.
src/hotspot/share/cds/filemap.cpp line 1099:
> 1097: lseek(_fd, _header->_base_archive_path_offset, SEEK_SET); //
> position to correct offset.
> 1098: size_t n = os::read(_fd, *target, (unsigned int)name_size);
> 1099: if (n != name_size) {
I think there's no need to do another read. The base name string is already
inside the buffer that was read on line 1079. You can just do a `strncpy` into
`*target`
src/hotspot/share/cds/filemap.cpp line 1205:
> 1203: }
> 1204:
> 1205: _header = (FileMapHeader*)os::malloc(gen_header->_header_size,
> mtInternal);
There's no need to allocate and read the header again. It's already in
gen_header. This should be enough:
_header = (FileMapHeader*)gen_header;
src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c line 369:
> 367:
> 368: // check file magic
> 369: if (header._generic_header._magic != CDS_ARCHIVE_MAGIC) {
Use `header.magic()` instead?
-
PR: https://git.openjdk.java.net/jdk/pull/5768
Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v4]
On Mon, 4 Oct 2021 17:50:27 GMT, Yumin Qi wrote:
>> Please review,
>> Refactor fundamental CDS FileMapHeader code for reliable reading of basic
>> info from shared archive.
>> With the change, it makes it possible to read an archive generated by
>> different version of hotspot. Also it is possible to automatically generate
>> a CDS archive If the archive supplied is not readable or fails to pass the
>> check.
>>
>> Tests: tier1-4
>>jtreg on sa.
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed comment
Changes requested by iklam (Reviewer).
src/hotspot/share/cds/filemap.cpp line 173:
> 171: memset((void*)this, 0, sizeof(FileMapInfo));
> 172: _is_static = is_static;
> 173: size_t c_header_size;
FileMapInfo::FileMapInfo() is called in two places:
(1) When reading an archive -- _header should not be initialized here. It
should be done inside FileMapInfo::init_from_file.
(2) When writing an archive -- I think we should move the _header
initialization to FileMapInfo::populate_header().
src/hotspot/share/cds/filemap.cpp line 197:
> 195: _header->set_base_archive_path_offset(0);
> 196: _header->set_base_archive_name_size(0);
> 197: _header->set_header_size((unsigned int)header_size);
The above 3 lines can be replaced by lines 203 .. 205, and 203 .. 205 can be
deleted.
src/hotspot/share/cds/filemap.cpp line 200:
> 198: _header->set_has_platform_or_app_classes(true);
> 199: if (!is_static) {
> 200:
> _header->set_base_archive_is_default(FLAG_IS_DEFAULT(SharedArchiveFile));
There's no need for a separate _base_archive_is_default field. We are using the
base archive if base_archive_name_size == 0.
src/hotspot/share/cds/filemap.cpp line 1057:
> 1055: GenericCDSFileMapHeader* header =
> (GenericCDSFileMapHeader*)os::malloc(sz, mtInternal);
> 1056: memset((void*)header, 0, sz);
> 1057: size_t n = os::read(fd, (void*)header, (unsigned int)sz);
There are currently 3 different places where we read the
GenericCDSFileMapHeader, find out the size, and then read the real header.
- FileMapInfo::get_base_archive_name_from_header
- FileMapInfo::check_archive
- FileMapInfo::init_from_file
I think these should be consolidated into a single function.
-
PR: https://git.openjdk.java.net/jdk/pull/5768
Re: RFR: 8273915: Create 'nosafepoint' rank [v3]
On Mon, 20 Sep 2021 20:16:03 GMT, Coleen Phillimore wrote: >> Partition safepoint checking and nonchecking lock ranks. The nonchecking >> locks are always lower ranked than the safepoint checking locks because they >> cannot block. >> >> This moves some leaf locks to 'nosafepoint' rank and corrects relative >> ranking. >> >> Tested with tier1-6 and built and run tier1 tests with shenandoah locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Add comment and change enum name to Rank. LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5550
Re: RFR: 8273915: Create 'nosafepoint' rank [v2]
On Mon, 20 Sep 2021 13:35:29 GMT, Coleen Phillimore wrote: >> Partition safepoint checking and nonchecking lock ranks. The nonchecking >> locks are always lower ranked than the safepoint checking locks because they >> cannot block. >> >> This moves some leaf locks to 'nosafepoint' rank and corrects relative >> ranking. >> >> Tested with tier1-6 and built and run tier1 tests with shenandoah locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Remove 'safepoint' rank, now unused. src/hotspot/share/runtime/mutex.hpp line 53: > 51:special= tty+ 3, > 52:oopstorage = special+ 3, > 53:nosafepoint= oopstorage + 6, Maybe add a comment below `nosafepoint` like: // A thread is not allowed to safepoint while holding a mutex whose // rank is nosafepoint or lower. Also, how about renaming the generic name `lock_types` to something specific like `standard_lock_ranks`? BTW, should we (in separate RFE) add a new enum `_safepoint_check_default`, so that this parameter can be omitted depending on the rank value (unless in places where you need to override it)? For one thing, I never understood which _safepoint_check_xxx I should have used when adding a new lock. I just randomly changed it until the JVM stops crashing. - PR: https://git.openjdk.java.net/jdk/pull/5550
Re: RFR: 8269537: memset() is called after operator new
On Tue, 7 Sep 2021 12:25:54 GMT, Leo Korinth wrote:
> The basic problem is that we are relying on undefined behaviour, as
> documented in the code:
>
> // This whole business of passing information from ResourceObj::operator new
> // to the ResourceObj constructor via fields in the "object" is technically
> UB.
> // But it seems to work within the limitations of HotSpot usage (such as no
> // multiple inheritance) with the compilers and compiler options we're using.
> // And it gives some possibly useful checking for misuse of ResourceObj.
>
>
> I am removing the undefined behaviour by passing the type of allocation
> through a thread local variable.
>
> This solution has some advantages:
> 1) it is not UB
> 2) it is simpler and easier to understand
> 3) it uses less memory (I could make it use even less if I made the enum
> `allocation_type` a u8)
> 4) in the *very* unlikely situation that stack memory (or embedded) already
> equals the data calculated from the address of the object, the code will also
> work.
>
> When doing the change, I also updated `allocated_on_stack()` to the new name
> `allocated_on_stack_or_embedded()` which is much harder to misinterpret.
>
> I also disallow to "fake" the memory type by explicitly calling
> `ResourceObj::set_allocation_type`.
>
> This forced me to change two places that is faking the allocation type of an
> embedded `GrowableArray` from `STACK_OR_EMBEDDED` to `C_HEAP`. The faking of
> the type is hard to understand as a `STACK_OR_EMBEDDED` `GrowableArray` can
> allocate any type of object. My guess is that `GrowableArray` has changed
> behaviour, or maybe that it was hard to understand because the old naming of
> `allocated_on_stack()`.
>
> I have also tried to update the comments. In doing that I not only changed
> the comments for this change, but also for the *incorrect* advice to always
> delete object you allocate with new.
>
> Testing on debug build tier1-3
> Testing on release build tier1
Changes requested by iklam (Reviewer).
src/hotspot/share/memory/allocation.hpp line 439:
> 437: void* operator new(size_t size, const std::nothrow_t&
> nothrow_constant) throw() {
> 438: address res = (address)resource_allocate_bytes(size,
> AllocFailStrategy::RETURN_NULL);
> 439: DEBUG_ONLY(if (res != NULL) _thread_last_allocated =
> RESOURCE_AREA;)
Maybe we should also guard against the possibility of nested allocations, which
may trash `_thread_last_allocated`?
#define PUSH_RESOURCE_OBJ_ALLOC_TYPE(t) \
assert(_thread_last_allocated == STACK_OR_EMBEDDED, "must not be nested"); \
DEBUG_ONLY(_thread_last_allocated = t); \
...
if (res != NULL) {
PUSH_RESOURCE_OBJ_ALLOC_TYPE(RESOURCE_AREA);
}
Similarly, the `ResourceObj` constructor should use a corresponding
`POP_RESOURCE_OBJ_ALLOC_TYPE` macro.
-
PR: https://git.openjdk.java.net/jdk/pull/5387
Re: RFR: 8269962: SA has unused Hashtable, Dictionary classes
On Wed, 7 Jul 2021 13:27:49 GMT, Coleen Phillimore wrote: > See bug for more details. This code is unused and is soon going to be not in > hotspot. > I left in logBytesPerWord() from my previous change because it might be > useful in the SA. I could remove it if opinion warrants. > Ran tier1-3. LGTM. Thanks for the cleanup. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4708
Re: RFR: 8252685: APIs that require JavaThread should take JavaThread arguments [v3]
On Thu, 13 May 2021 04:59:11 GMT, David Holmes wrote: >> Our code is littered with API's that take, or manifest, a Thread* and then >> assert/guarantee that it must be a JavaThread, rather than >> taking/manifesting a JavaThread in the first place. The main reason for this >> is that the TRAPS macro, used in relation to exception generation and >> processing, is declared as "Thread* THREAD". In practice only JavaThreads >> that can execute Java code can generate arbitrary exceptions, because it >> requires executing Java code. In other places we can get away with other >> kinds of threads "throwing" exceptions because they are only pre-allocated >> instances that require no Java code execution (e.g. OOME), or when executed >> by a non-JavaThread the code actually by-passes the logic would throw an >> exception. Such code also eventually clears the exception and reports the >> OOM by some other means. We have been progressively untangling these code >> paths and modifying TRAPS/CHECK usage so that only JavaThreads will call >> TRAPS methods and throw exceptions. Having done t hat pre-work we are now ready to convert TRAPS to be "JavaThread* THREAD" and that is what this change does. The resulting changes are largely mechanical: >> >> - declarations of "Thread* THREAD" become "JavaThread* THREAD" (where THREAD >> is needed for exceptions, otherwise THREAD is renamed to current) >> - methods that took a Thread* parameter that was always THREAD, now take a >> JavaThread* param >> - Manifestations of Thread::current() become JavaThread::current() >> - THREAD->as_Java_thread() becomes just THREAD >> - THREAD->is_Java_thread() is reworked so that THREAD is "current" >> >> There are still places where a CompilerThread (which is a JavaThread but may >> not be able to execute Java code) calls a TRAPS function (primarily where >> OOME is possible). Fixing that would be a future RFE and may not be possible >> without reworking a lot of the allocation code paths. >> >> Testing: >> - tiers 1-8 on Linux-x64 >> - all builds in tiers 1-4 >> - tiers 1-3 on all platforms >> >> Thanks, >> David > > David Holmes has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Update after merge with master > - Merge branch 'master' into 8252685-JavaThread > - Review feedback from Serguei > - Merge > - 8252685: APIs that require JavaThread should take JavaThread arguments LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3877
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
On Tue, 11 May 2021 14:13:49 GMT, Harold Seigel wrote: >> Please review this large change to remove Unsafe::defineAnonymousClass(). >> The change removes dAC relevant code and changes a lot of tests. Many of >> the changed tests need renaming. I hope to do this in a follow up RFE. >> Some of the tests were modified to use hidden classes, others were deleted >> because either similar hidden classes tests already exist or they tested dAC >> specific functionality, such as host classes. >> >> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, >> and Mach5 tiers 3-7 on Linux x64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > fix GetModuleTest.java The CDS VM and test changes look OK to me. src/hotspot/share/oops/instanceMirrorKlass.inline.hpp line 65: > 63: // so when handling the java mirror for the class we need to make > sure its class > 64: // loader data is claimed, this is done by calling do_cld > explicitly. > 65: // For non-string hidden classes the call to do_cld is made when > the class Typo: non-strong - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8266497: Remove unnecessary EMCP liveness indication
On Tue, 4 May 2021 12:31:46 GMT, Coleen Phillimore wrote: > Marking running_emcp for Method* is unnecessary. We can set/clear > breakpoints in all old emcp methods because they're not deallocated until > none are running. See longer explanation in the CR. > > Tested with tier1-6, tiers 7,8 are running and 98% passing. Looks reasonable to me. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3852
Re: RFR: 8259070: Add jcmd option to dump CDS [v11]
On Tue, 13 Apr 2021 23:04:24 GMT, Yumin Qi wrote: >> Hi, Please review >> >> Added jcmd option for dumping CDS archive during application runtime. >> Before this change, user has to dump shared archive in two steps: first run >> application with >> `java -XX:DumpLoadedClassList= ` >> to collect shareable class names and saved in file `` , then >> `java -Xshare:dump -XX:SharedClassListFile= >> -XX:SharedArchiveFile= ...` >> With this change, user can use jcmd to dump CDS without going through >> above steps. Also user can choose a moment during the app runtime to dump >> an archive. >>The bug is associated with the CSR: >> https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved. >>New added jcmd option: >>`jcmd VM.cds static_dump ` >>or >> `jcmd VM.cds dynamic_dump ` >> To dump dynamic archive, requires start app with newly added flag >> `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to >> dynamic dump like loader constraints will be recorded. Note the dumping >> process changed some object memory locations so for dumping dynamic archive, >> can only done once for a running app. For static dump, user can dump >> multiple times against same process. >>The file name is optional, if the file name is not supplied, the file >> name will take format of `java_pid_static.jsa` or >> `java_pid_dynamic.jsa` for static and dynamic respectively. The >> `` is the application process ID. >> >> Tests: tier1,tier2,tier3,tier4 >> >> Thanks >> Yumin > > Yumin Qi has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Fix too much verbose output, make call to stopApp after test > - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070 > - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070 > - Restructed code and rename LingeredTestApp.java to JCmdTestLingeredApp.java > - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070 > - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070 > - Fix revert unintentionally comment, merge master. > - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070 > - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. > Removed unused function from ClassLoader. Improved > InstanceKlass::is_shareable() and related test. Added more test scenarios. > - Remove redundant check for if a class is shareable > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/008fc75a...88c0f7d1 The latest versions looks good to me. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2737
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:54:51 GMT, Ioi Lam wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove exports from Graal module to jdk.aot > > LGTM. Just a small nit. > @iklam > I thought the fingerprint code was also used by CDS. CDS actually uses a different mechanism (CRC of the classfile bytes) to validate that a class has not changed between archive dumping time and runtime. See https://github.com/openjdk/jdk/blob/5784f6b7f74d0b8081ac107fea172539d57d6020/src/hotspot/share/classfile/systemDictionaryShared.cpp#L1126-L1130 - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov wrote:
>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related
>> to Ahead-of-Time Compiler from JDK:
>>
>> - `jdk.aot` module — the `jaotc` tool
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution
>> - related HotSpot code guarded by `#if INCLUDE_AOT`
>>
>> Additionally, remove tests as well as code in makefiles related to AoT
>> compilation.
>>
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove exports from Graal module to jdk.aot
LGTM. Just a small nit.
src/hotspot/share/oops/methodCounters.cpp line 77:
> 75: }
> 76:
> 77: void MethodCounters::metaspace_pointers_do(MetaspaceClosure* it) {
MethodCounters::metaspace_pointers_do can be removed altogether (also need to
remove the declaration in methodCounter.hpp).
-
Marked as reviewed by iklam (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3398
Integrated: 8264285: Clean the modification of ccstr JVM flags
On Mon, 29 Mar 2021 21:35:52 GMT, Ioi Lam wrote: > There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM flags > of the ccstr type (i.e., strings). > > - One version requires the caller to free the old value, but some callers > don't do that (writeableFlags.cpp). > - The other version frees the old value on behalf of the caller. However, > this version is accessible only via FLAG_SET_XXX macros and is currently > unused. So it's unclear whether it actually works. > > We should combine these two versions into a single function, fix problems in > the callers, and add test cases. The old value should be freed automatically, > because typically the caller isn't interested in the old value. > > Note that the FLAG_SET_XXX macros do not return the old value. Requiring the > caller of FLAG_SET_XXX to free the old value would be tedious and error prone. This pull request has now been integrated. Changeset: 58583990 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/58583990 Stats: 205 lines in 9 files changed: 160 ins; 24 del; 21 mod 8264285: Clean the modification of ccstr JVM flags Reviewed-by: dholmes, coleenp - PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]
On Thu, 1 Apr 2021 00:25:59 GMT, Coleen Phillimore wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> relax flag attributions (ala JDK-7123237) > > Marked as reviewed by coleenp (Reviewer). Thanks @coleenp and @dholmes-ora for reviewing. - PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v4]
> There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM flags > of the ccstr type (i.e., strings). > > - One version requires the caller to free the old value, but some callers > don't do that (writeableFlags.cpp). > - The other version frees the old value on behalf of the caller. However, > this version is accessible only via FLAG_SET_XXX macros and is currently > unused. So it's unclear whether it actually works. > > We should combine these two versions into a single function, fix problems in > the callers, and add test cases. The old value should be freed automatically, > because typically the caller isn't interested in the old value. > > Note that the FLAG_SET_XXX macros do not return the old value. Requiring the > caller of FLAG_SET_XXX to free the old value would be tedious and error prone. Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into 8264285-clean-up-setting-ccstr-jvm-flags - Revert "relax flag attributions (ala JDK-7123237)" This reverts commit 673aaafc4860dd7f70a3f222422ae84e85fd4219. - relax flag attributions (ala JDK-7123237) - restored SET_FLAG_XXX for ccstr type, and fixed bugs in existing ccstr modification code - 8264285: Do not support FLAG_SET_XXX for VM flags of string type - Changes: - all: https://git.openjdk.java.net/jdk/pull/3254/files - new: https://git.openjdk.java.net/jdk/pull/3254/files/2a5e2b19..5f912221 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3254=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3254=02-03 Stats: 8053 lines in 376 files changed: 5492 ins; 1012 del; 1549 mod Patch: https://git.openjdk.java.net/jdk/pull/3254.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3254/head:pull/3254 PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8264150: CDS dumping code calls TRAPS functions in VM thread
On Tue, 30 Mar 2021 22:25:06 GMT, Coleen Phillimore wrote: > This change initializes the vtables/itables without checking constraints. > After initializing but before the class is visible in the SystemDictionary, > constraints are checked in a separate loop. > > Tested with tier1-6 which includes jck tests. Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3277
Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]
On 3/31/21 10:47 PM, David Holmes wrote:
On 1/04/2021 3:29 pm, Ioi Lam wrote:
On 3/31/21 10:22 PM, David Holmes wrote:
On 1/04/2021 5:05 am, Ioi Lam wrote:
On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore
wrote:
36: // have any MANAGEABLE flags of the ccstr type, but we really
need to
37: // make sure the implementation is correct (in terms of
memory allocation)
38: // just in case someone may add such a flag in the future.
Could you have just added a develop flag to the manageable flags
instead?
I had to use a `product` flag due to the following code, which
should have been removed as part of
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208),
but I was afraid to do so because I didn't have a test case. I.e.,
all of our diagnostic/manageable/experimental flags were `product`
flags.
With this PR, now I have a test case -- I changed
`DummyManageableStringFlag` to a `notproduct` flag, and removed the
following code. I am re-running tiers1-4 now.
Sorry but I don't understand how a test involving one flag replaces
a chunk of code that validated every flag declaration ??
When I did JDK-8243208, I wasn't sure if the VM would actually
support diagnostic/manageable/experimental flags that were not
`product`. So I added check_all_flag_declarations() to prevent anyone
from adding such a flag "casually" without thinking about.
Now that I have added such a flag, and I believe I have tested pretty
thoroughly (tiers 1-4), I think the VM indeed supports these flags.
So now I feel it's safe to remove check_all_flag_declarations().
But the check was checking two things:
1. That diagnostic/manageable/experimental are mutually exclusive
2. That they only apply to product flags
I believe both of these rules still stand based on what we defined
such attributes to mean. And even if you think #2 should not apply, #1
still does and so could still be checked. And if #2 is no longer our
rule then the documentation in globals.hpp should be updated - though
IMHO #2 should remain as-is.
I think neither #1 and #2 make sense. These were limitation introduced
by the old flags implementation, where you had to declare a flag using
one of these macros
diagnostic(type, name,
manageable(type, name,
experimental(type, name,
That's why you have #1 (mutual exclusion).
#2 was also due to the implementation. It makes no sense that you can't
have an develop flag for an experimental feature.
However, in the old implementation, adding more variations would cause
macro explosion. See
https://github.com/openjdk/jdk/blame/7d8519fffe46b6b5139b3aa51b18fcf0249a9e14/src/hotspot/share/runtime/flags/jvmFlag.cpp#L776
Anyway, changing this should be done in a separate RFE. I have reverted
[v2] from this PR, so we are back to [v1].
Thanks
- Ioi
David
-
Thanks
- Ioi
David
-
void JVMFlag::check_all_flag_declarations() {
for (JVMFlag* current = [0]; current->_name != NULL;
current++) {
int flags = static_cast(current->_flags);
// Backwards compatibility. This will be relaxed/removed in
JDK-7123237.
int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE
| JVMFlag::KIND_EXPERIMENTAL;
if ((flags & mask) != 0) {
assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
(flags & mask) == JVMFlag::KIND_MANAGEABLE ||
(flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
"%s can be declared with at most one of "
"DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL",
current->_name);
assert((flags & KIND_NOT_PRODUCT) == 0 &&
(flags & KIND_DEVELOP) == 0,
"%s has an optional DIAGNOSTIC, MANAGEABLE or
EXPERIMENTAL "
"attribute; it must be declared as a product flag",
current->_name);
}
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v3]
> There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM flags > of the ccstr type (i.e., strings). > > - One version requires the caller to free the old value, but some callers > don't do that (writeableFlags.cpp). > - The other version frees the old value on behalf of the caller. However, > this version is accessible only via FLAG_SET_XXX macros and is currently > unused. So it's unclear whether it actually works. > > We should combine these two versions into a single function, fix problems in > the callers, and add test cases. The old value should be freed automatically, > because typically the caller isn't interested in the old value. > > Note that the FLAG_SET_XXX macros do not return the old value. Requiring the > caller of FLAG_SET_XXX to free the old value would be tedious and error prone. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: Revert "relax flag attributions (ala JDK-7123237)" This reverts commit 673aaafc4860dd7f70a3f222422ae84e85fd4219. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3254/files - new: https://git.openjdk.java.net/jdk/pull/3254/files/673aaafc..2a5e2b19 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3254=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3254=01-02 Stats: 37 lines in 4 files changed: 36 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3254.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3254/head:pull/3254 PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]
On 3/31/21 10:22 PM, David Holmes wrote:
On 1/04/2021 5:05 am, Ioi Lam wrote:
On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore
wrote:
36: // have any MANAGEABLE flags of the ccstr type, but we really
need to
37: // make sure the implementation is correct (in terms of memory
allocation)
38: // just in case someone may add such a flag in the future.
Could you have just added a develop flag to the manageable flags
instead?
I had to use a `product` flag due to the following code, which should
have been removed as part of
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but
I was afraid to do so because I didn't have a test case. I.e., all of
our diagnostic/manageable/experimental flags were `product` flags.
With this PR, now I have a test case -- I changed
`DummyManageableStringFlag` to a `notproduct` flag, and removed the
following code. I am re-running tiers1-4 now.
Sorry but I don't understand how a test involving one flag replaces a
chunk of code that validated every flag declaration ??
When I did JDK-8243208, I wasn't sure if the VM would actually support
diagnostic/manageable/experimental flags that were not `product`. So I
added check_all_flag_declarations() to prevent anyone from adding such a
flag "casually" without thinking about.
Now that I have added such a flag, and I believe I have tested pretty
thoroughly (tiers 1-4), I think the VM indeed supports these flags. So
now I feel it's safe to remove check_all_flag_declarations().
Thanks
- Ioi
David
-
void JVMFlag::check_all_flag_declarations() {
for (JVMFlag* current = [0]; current->_name != NULL;
current++) {
int flags = static_cast(current->_flags);
// Backwards compatibility. This will be relaxed/removed in
JDK-7123237.
int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE |
JVMFlag::KIND_EXPERIMENTAL;
if ((flags & mask) != 0) {
assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
(flags & mask) == JVMFlag::KIND_MANAGEABLE ||
(flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
"%s can be declared with at most one of "
"DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
assert((flags & KIND_NOT_PRODUCT) == 0 &&
(flags & KIND_DEVELOP) == 0,
"%s has an optional DIAGNOSTIC, MANAGEABLE or
EXPERIMENTAL "
"attribute; it must be declared as a product flag",
current->_name);
}
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8259070: Add jcmd option to dump CDS [v8]
On Wed, 31 Mar 2021 20:58:46 GMT, Yumin Qi wrote:
>> Hi, Please review
>>
>> Added jcmd option for dumping CDS archive during application runtime.
>> Before this change, user has to dump shared archive in two steps: first run
>> application with
>> `java -XX:DumpLoadedClassList= `
>> to collect shareable class names and saved in file `` , then
>> `java -Xshare:dump -XX:SharedClassListFile=
>> -XX:SharedArchiveFile= ...`
>> With this change, user can use jcmd to dump CDS without going through
>> above steps. Also user can choose a moment during the app runtime to dump
>> an archive.
>>The bug is associated with the CSR:
>> https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>New added jcmd option:
>>`jcmd VM.cds static_dump `
>>or
>> `jcmd VM.cds dynamic_dump `
>> To dump dynamic archive, requires start app with newly added flag
>> `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to
>> dynamic dump like loader constraints will be recorded. Note the dumping
>> process changed some object memory locations so for dumping dynamic archive,
>> can only done once for a running app. For static dump, user can dump
>> multiple times against same process.
>>The file name is optional, if the file name is not supplied, the file
>> name will take format of `java_pid_static.jsa` or
>> `java_pid_dynamic.jsa` for static and dynamic respectively. The
>> `` is the application process ID.
>>
>> Tests: tier1,tier2,tier3,tier4
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 10 commits:
>
> - Fix revert unintentionally comment, merge master.
> - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
> - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments.
> Removed unused function from ClassLoader. Improved
> InstanceKlass::is_shareable() and related test. Added more test scenarios.
> - Remove redundant check for if a class is shareable
> - Fix according to review comment and add more tests
> - Fix filter more flags to exclude in static dump, add more test cases
> - Merge branch 'master' into jdk-8259070
> - Fix white space in CDS.java
> - Add function CDS.dumpSharedArchive in java to dump shared archive
> - 8259070: Add jcmd option to dump CDS
Changes requested by iklam (Reviewer).
src/hotspot/share/services/diagnosticCommand.cpp line 1106:
> 1104: output()->print_cr("Dynamic dump:");
> 1105: if (!UseSharedSpaces) {
> 1106: output()->print_cr("CDS is not available for the JDK");
I think we should use a message similar to this:
$ java -Xshare:off -XX:ArchiveClassesAtExit=xxx -version
Error occurred during initialization of VM
DynamicDumpSharedSpaces is unsupported when base CDS archive is not loaded
How about "dynamic_dump is unsupported when base CDS archive is not loaded".
src/hotspot/share/services/diagnosticCommand.cpp line 1125:
> 1123: Symbol* cds_name = vmSymbols::jdk_internal_misc_CDS();
> 1124: Klass* cds_klass = SystemDictionary::resolve_or_fail(cds_name, true
> /*throw error*/, CHECK);
> 1125: JavaValue result(T_OBJECT);
Should be `result(T_VOID)` to match the signature of `CDS.dumpSharedArchive()`
src/hotspot/share/services/diagnosticCommand.cpp line 1133:
> 1131: vmSymbols::dumpSharedArchive(),
> 1132: vmSymbols::dumpSharedArchive_signature(),
> 1133: , THREAD);
Should be `, CHECK);`. Recently, we have started to to avoid using
`THREAD` if the callee function may throw an exception.
src/java.base/share/classes/jdk/internal/misc/CDS.java line 218:
> 216: try {
> 217: PrintStream prt = new PrintStream(fileName);
> 218: prt.println("Command:");
[Try-with-resources](https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html)
should be used to make sure the streams are closed when the method exits
(normally or on exception).
try (InputStreamReader isr = new InputStreamReader(stream);
BufferedReader rdr = new BufferedReader(isr);
PrintStream prt = new PrintStream(fileName);)
{
prt.println("Command:");
src/java.base/share/classes/jdk/internal/misc/CDS.java line 307:
> 305: outputStdStream(proc.getErrorStream(), stdErrFile,
> cmds);
> 306: }).start();
> 307:
I think the above code can be refactored to avoid duplication. Something like:
String stdOutFile = drainOutput(proc.getInputStream(), "stdout");
String stdErrFile = drainOutput(proc.getErrorStream(), "stderr");
and move the common code into drainOutput().
-
PR: https://git.openjdk.java.net/jdk/pull/2737
Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]
On Thu, 1 Apr 2021 00:25:53 GMT, Coleen Phillimore wrote:
>> I had to use a `product` flag due to the following code, which should have
>> been removed as part of
>> [JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but I was
>> afraid to do so because I didn't have a test case. I.e., all of our
>> diagnostic/manageable/experimental flags were `product` flags.
>>
>> With this PR, now I have a test case -- I changed
>> `DummyManageableStringFlag` to a `notproduct` flag, and removed the
>> following code. I am re-running tiers1-4 now.
>>
>> void JVMFlag::check_all_flag_declarations() {
>> for (JVMFlag* current = [0]; current->_name != NULL; current++) {
>> int flags = static_cast(current->_flags);
>> // Backwards compatibility. This will be relaxed/removed in JDK-7123237.
>> int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE |
>> JVMFlag::KIND_EXPERIMENTAL;
>> if ((flags & mask) != 0) {
>> assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
>> (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
>> (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
>> "%s can be declared with at most one of "
>> "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
>> assert((flags & KIND_NOT_PRODUCT) == 0 &&
>> (flags & KIND_DEVELOP) == 0,
>> "%s has an optional DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL "
>> "attribute; it must be declared as a product flag",
>> current->_name);
>> }
>> }
>> }
>
> What's the difference between notproduct and develop again? Do we run tests
> with the optimized build and why would this flag be available in that build?
> ie. why not develop?
>From the top of globals.hpp:
- `develop` flags are settable / visible only during development and are
constant in the PRODUCT version
- `notproduct` flags are settable / visible only during development and are not
declared in the PRODUCT version
Since this flag is only used in test cases, and specifically for modifying its
value, it doesn't make sense to expose this flag to the PRODUCT version at all.
So I chose `notproduct`, so we can save a few bytes for the PRODUCT as well.
-
PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8259070: Add jcmd option to dump CDS [v8]
On Wed, 31 Mar 2021 20:58:46 GMT, Yumin Qi wrote:
>> Hi, Please review
>>
>> Added jcmd option for dumping CDS archive during application runtime.
>> Before this change, user has to dump shared archive in two steps: first run
>> application with
>> `java -XX:DumpLoadedClassList= `
>> to collect shareable class names and saved in file `` , then
>> `java -Xshare:dump -XX:SharedClassListFile=
>> -XX:SharedArchiveFile= ...`
>> With this change, user can use jcmd to dump CDS without going through
>> above steps. Also user can choose a moment during the app runtime to dump
>> an archive.
>>The bug is associated with the CSR:
>> https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>New added jcmd option:
>>`jcmd VM.cds static_dump `
>>or
>> `jcmd VM.cds dynamic_dump `
>> To dump dynamic archive, requires start app with newly added flag
>> `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to
>> dynamic dump like loader constraints will be recorded. Note the dumping
>> process changed some object memory locations so for dumping dynamic archive,
>> can only done once for a running app. For static dump, user can dump
>> multiple times against same process.
>>The file name is optional, if the file name is not supplied, the file
>> name will take format of `java_pid_static.jsa` or
>> `java_pid_dynamic.jsa` for static and dynamic respectively. The
>> `` is the application process ID.
>>
>> Tests: tier1,tier2,tier3,tier4
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 10 commits:
>
> - Fix revert unintentionally comment, merge master.
> - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
> - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments.
> Removed unused function from ClassLoader. Improved
> InstanceKlass::is_shareable() and related test. Added more test scenarios.
> - Remove redundant check for if a class is shareable
> - Fix according to review comment and add more tests
> - Fix filter more flags to exclude in static dump, add more test cases
> - Merge branch 'master' into jdk-8259070
> - Fix white space in CDS.java
> - Add function CDS.dumpSharedArchive in java to dump shared archive
> - 8259070: Add jcmd option to dump CDS
Changes requested by iklam (Reviewer).
src/hotspot/share/classfile/vmSymbols.hpp line 304:
> 302: template(generateLambdaFormHolderClasses_signature,
> "([Ljava/lang/String;)[Ljava/lang/Object;") \
> 303: template(dumpSharedArchive, "dumpSharedArchive")
> \
> 304: template(dumpSharedArchive_signature, "(ZLjava/lang/String;)V")
> \
Need to align the "dumpSharedArchive" part with the previous line.
src/hotspot/share/prims/jvm.cpp line 3745:
> 3743: #if INCLUDE_CDS
> 3744: assert(UseSharedSpaces && RecordDynamicDumpInfo, "already checked in
> arguments.cpp?");
> 3745: if (DynamicArchive::has_been_dumped_once()) {
Maybe add a comment like this:?
// During dynamic archive dumping, some of the data structures are overwritten
so
// we cannot dump the dynamic archive again. TODO: this should be fixed.
src/hotspot/share/prims/jvm.cpp line 3754:
> 3752: assert(ArchiveClassesAtExit == nullptr, "already checked in
> arguments.cpp?");
> 3753: Handle file_handle(THREAD, JNIHandles::resolve_non_null(archiveName));
> 3754: char* archive_name = java_lang_String::as_utf8_string(file_handle());
A ResourceMark is needed before calling java_lang_String::as_utf8_string().
In general, I think the code in jvm.cpp should only marshall the jobject
argument (e.g., convert `jstring` to `char*`.). The main functionality of
JVM_DumpDynamicArchive should be moved to dynamicArchive.cpp. Similarly, most
of the work in JVM_DumpClassListToFile should be moved to metaspaceShared.cpp.
src/hotspot/share/prims/jvm.cpp line 3759:
> 3757: DynamicArchive::dump();
> 3758: } else {
> 3759: THROW_MSG(vmSymbols::java_lang_RuntimeException(),
Need to set ArchiveClassesAtExit to NULL before throwing the exception, since
dynamic dump may not work anymore after the failure.
test/hotspot/jtreg/runtime/cds/appcds/jcmd/LingeredTestApp.java line 28:
> 26: public class LingeredTestApp extends LingeredApp {
> 27: // Do not use default test.class.path in class path.
> 28: public boolean useDefaultClasspath() { return false; }
It's not obvious that you're changing the behavior of the base class by
overriding a member function. It's better to have
public LingeredTestApp() {
setUseDefaultClasspath(false);
}
Also, the name of LingeredTestApp is kind of generic. How about renaming it to
JCmdTestLingeredApp?
-
PR: https://git.openjdk.java.net/jdk/pull/2737
Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]
On Tue, 30 Mar 2021 03:44:26 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> relax flag attributions (ala JDK-7123237)
>
> src/hotspot/share/services/writeableFlags.cpp line 250:
>
>> 248: if (err == JVMFlag::SUCCESS) {
>> 249: assert(value == NULL, "old value is freed automatically and not
>> returned");
>> 250: }
>
> The whole block should be ifdef DEBUG.
Since this whole block can be optimized out by the C compiler in product
builds, I'd rather leave out the `#ifdef` to avoid clutter.
-
PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]
On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> relax flag attributions (ala JDK-7123237)
>
> src/hotspot/share/runtime/flags/debug_globals.hpp line 38:
>
>> 36: // have any MANAGEABLE flags of the ccstr type, but we really need to
>> 37: // make sure the implementation is correct (in terms of memory
>> allocation)
>> 38: // just in case someone may add such a flag in the future.
>
> Could you have just added a develop flag to the manageable flags instead?
I had to use a `product` flag due to the following code, which should have been
removed as part of
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but I was
afraid to do so because I didn't have a test case. I.e., all of our
diagnostic/manageable/experimental flags were `product` flags.
With this PR, now I have a test case -- I changed `DummyManageableStringFlag`
to a `notproduct` flag, and removed the following code. I am re-running
tiers1-4 now.
void JVMFlag::check_all_flag_declarations() {
for (JVMFlag* current = [0]; current->_name != NULL; current++) {
int flags = static_cast(current->_flags);
// Backwards compatibility. This will be relaxed/removed in JDK-7123237.
int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE |
JVMFlag::KIND_EXPERIMENTAL;
if ((flags & mask) != 0) {
assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
(flags & mask) == JVMFlag::KIND_MANAGEABLE ||
(flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
"%s can be declared with at most one of "
"DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
assert((flags & KIND_NOT_PRODUCT) == 0 &&
(flags & KIND_DEVELOP) == 0,
"%s has an optional DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL "
"attribute; it must be declared as a product flag",
current->_name);
}
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]
> There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM flags > of the ccstr type (i.e., strings). > > - One version requires the caller to free the old value, but some callers > don't do that (writeableFlags.cpp). > - The other version frees the old value on behalf of the caller. However, > this version is accessible only via FLAG_SET_XXX macros and is currently > unused. So it's unclear whether it actually works. > > We should combine these two versions into a single function, fix problems in > the callers, and add test cases. The old value should be freed automatically, > because typically the caller isn't interested in the old value. > > Note that the FLAG_SET_XXX macros do not return the old value. Requiring the > caller of FLAG_SET_XXX to free the old value would be tedious and error prone. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: relax flag attributions (ala JDK-7123237) - Changes: - all: https://git.openjdk.java.net/jdk/pull/3254/files - new: https://git.openjdk.java.net/jdk/pull/3254/files/7eca2343..673aaafc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3254=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3254=00-01 Stats: 37 lines in 4 files changed: 0 ins; 36 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3254.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3254/head:pull/3254 PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8259070: Add jcmd option to dump CDS [v7]
On Tue, 30 Mar 2021 03:48:08 GMT, Yumin Qi wrote:
>> Hi, Please review
>>
>> Added jcmd option for dumping CDS archive during application runtime.
>> Before this change, user has to dump shared archive in two steps: first run
>> application with
>> `java -XX:DumpLoadedClassList= `
>> to collect shareable class names and saved in file `` , then
>> `java -Xshare:dump -XX:SharedClassListFile=
>> -XX:SharedArchiveFile= ...`
>> With this change, user can use jcmd to dump CDS without going through
>> above steps. Also user can choose a moment during the app runtime to dump
>> an archive.
>>The bug is associated with the CSR:
>> https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>New added jcmd option:
>>`jcmd VM.cds static_dump `
>>or
>> `jcmd VM.cds dynamic_dump `
>> To dump dynamic archive, requires start app with newly added flag
>> `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to
>> dynamic dump like loader constraints will be recorded. Note the dumping
>> process changed some object memory locations so for dumping dynamic archive,
>> can only done once for a running app. For static dump, user can dump
>> multiple times against same process.
>>The file name is optional, if the file name is not supplied, the file
>> name will take format of `java_pid_static.jsa` or
>> `java_pid_dynamic.jsa` for static and dynamic respectively. The
>> `` is the application process ID.
>>
>> Tests: tier1,tier2,tier3,tier4
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. Removed
> unused function from ClassLoader. Improved InstanceKlass::is_shareable() and
> related test. Added more test scenarios.
src/java.base/share/classes/jdk/internal/misc/CDS.java line 311:
> 309: // done, delete classlist file.
> 310: if (fileList.exists()) {
> 311: // fileList.delete();
Is the comment unintentional?
-
PR: https://git.openjdk.java.net/jdk/pull/2737
RFR: 8264285: Clean the modification of ccstr JVM flags
There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM flags of the ccstr type (i.e., strings). - One version requires the caller to free the old value, but some callers don't do that (writeableFlags.cpp). - The other version frees the old value on behalf of the caller. However, this version is accessible only via FLAG_SET_XXX macros and is currently unused. So it's unclear whether it actually works. We should combine these two versions into a single function, fix problems in the callers, and add test cases. The old value should be freed automatically, because typically the caller isn't interested in the old value. Note that the FLAG_SET_XXX macros do not return the old value. Requiring the caller of FLAG_SET_XXX to free the old value would be tedious and error prone. - Commit messages: - restored SET_FLAG_XXX for ccstr type, and fixed bugs in existing ccstr modification code - 8264285: Do not support FLAG_SET_XXX for VM flags of string type Changes: https://git.openjdk.java.net/jdk/pull/3254/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3254=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264285 Stats: 205 lines in 9 files changed: 160 ins; 24 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/3254.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3254/head:pull/3254 PR: https://git.openjdk.java.net/jdk/pull/3254
Re: RFR: 8264004: Don't use TRAPS if no exceptions are thrown [v5]
On Wed, 24 Mar 2021 02:10:53 GMT, Coleen Phillimore wrote: >> Removed the TRAPS in function declarations in jvmtiRedefineClasses and in >> ConstantPool merging functions. >> Tested with vmTestbase/nsk/jvmti and tier1 (in progress). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Add ExceptionMarks. Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3141
Re: RFR: 8264004: Don't use TRAPS if no exceptions are thrown [v4]
On Tue, 23 Mar 2021 16:08:59 GMT, Coleen Phillimore wrote: >> Removed the TRAPS in function declarations in jvmtiRedefineClasses and in >> ConstantPool merging functions. >> Tested with vmTestbase/nsk/jvmti and tier1 (in progress). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix load_new_class_versions and remove more traps. * THREAD = current; // for exception processing``` is used only when the current method does not declare TRAPS, which means it should never throw. As a convention, I think the above code should be accompanied with by em(THREAD);``` to ensure that the exceptions are not unintentionally leaked. src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 1389: > 1387: state->set_class_being_redefined(the_class, _class_load_kind); > 1388: > 1389: Thread* THREAD = current; // for exception processing Add `ExceptionMark em(THREAD);` src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2118: > 2116: methodHandle method(current, methods->at(i)); > 2117: methodHandle new_method; > 2118: Thread* THREAD = current; // For exception handling Add `ExceptionMark em(THREAD);` - Changes requested by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3141
Re: RFR: 8264051: Remove unused TRAPS parameters from runtime functions
On Tue, 23 Mar 2021 16:40:44 GMT, Coleen Phillimore wrote: > This change removes the TRAPS parameter from compute_modifier_flags(), > lookup_instance_method_in_klasses and nest_host_error. > > There's a progressive effort to remove cases where the last parameter of a > function is THREAD, and it's unclear why it is ignoring an exception or > whether an exception is expected, if it doesn't subsequently have a check for > HAS_PENDING_EXCEPTION. > > Tested locally with tier1 tests and tier1 tests on 4 Oracle platforms in > progress. LGTM. Thanks for doing the cleanup. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3157
Re: RFR: 8264004: Don't use TRAPS if no exceptions are thrown [v2]
On Tue, 23 Mar 2021 01:22:56 GMT, Coleen Phillimore wrote: >> Removed the TRAPS in function declarations in jvmtiRedefineClasses and in >> ConstantPool merging functions. >> Tested with vmTestbase/nsk/jvmti and tier1 (in progress). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > missed THREAD that should be CHECK_false argument. LGTM. I think suggested TRAPS changes could be done in a separate REF. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3141
Re: RFR: 8264004: Don't use TRAPS if no exceptions are thrown [v2]
On Tue, 23 Mar 2021 04:59:10 GMT, David Holmes wrote: >> Coleen Phillimore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> missed THREAD that should be CHECK_false argument. > > src/hotspot/share/prims/jvmtiRedefineClasses.hpp line 484: > >> 482: void rewrite_cp_refs_in_method(methodHandle method, >> 483: methodHandle * new_method_p, TRAPS); >> 484: bool rewrite_cp_refs_in_methods(InstanceKlass* scratch_class, TRAPS); > > This method clears any pending exception and so should not be a TRAPS method. `VM_RedefineClasses::load_new_class_versions` also seems to never throw. These functions should be changed to take a `Thread*` parameter, and should use `HandleMark em(thread);` to guarantee that an exception never leaves the function. - PR: https://git.openjdk.java.net/jdk/pull/3141
