Re: RFR: 8319873: Add windows implementation for jcmd System.map and System.dump_map
On Thu, 15 Aug 2024 13:45:16 GMT, Simon Tooke wrote:
> This is a port of [JDK-8318636](https://github.com/openjdk/jdk/pull/16301) to
> Windows.
>
> System.map and System.dump_map are implemented using the Windows API and
> provide roughly the same information in the same format. Most of the heavy
> lifting was implemented by @tstuefe in #16301 - this PR adds the Windows
> implementation and enables the common code for Windows 64 bit.
>
> [Sample output (with NMT
> enabled)](https://github.com/user-attachments/files/16663332/vm_memory_map_760.txt)
Hi Simon,
this is nice, thank you!
Mostly nits.
src/hotspot/os/windows/memMapPrinter_windows.cpp line 3:
> 1: /*
> 2: * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights
> reserved.
> 3: * Copyright (c) 2023, 2024, Red Hat, Inc. and/or its affiliates.
New files are added with the current copyright year, unless they are the result
of moving.
src/hotspot/os/windows/memMapPrinter_windows.cpp line 32:
> 30:
> 31: #include
> 32: #include
Please use C-headers. We only use C++ headers sparingly. Why do you include
these headers?
src/hotspot/os/windows/memMapPrinter_windows.cpp line 63:
> 61: }
> 62:
> 63: const char* getProtectString(char* buffer, size_t bufsiz, DWORD prot)
> {
Style: In hotspot we use camel case for classes, lower_case_underscore for
method, functions etc.
src/hotspot/os/windows/memMapPrinter_windows.cpp line 69:
> 67: const int IP = 3;
> 68: int idx = 4;
> 69: strncpy_s(buffer, bufsiz, " ", 7);
We tend to use os::snprintf or jio_snprintf for this. I find strncpy_s a bit
fragile since one needs to update both string literal and char count when
changing things.
src/hotspot/os/windows/memMapPrinter_windows.cpp line 98:
> 96: buffer[idx++] = 'n';
> 97: } else if (prot != 0) {
> 98: snprintf(buffer, bufsiz, "(0x%x)", prot);
This could truncate for the longest possible value (16+4 chars).
src/hotspot/os/windows/memMapPrinter_windows.cpp line 111:
> 109: buffer[idx++] = 'W';
> 110: }
> 111: buffer[idx] = 0;
Please give us an assert here for idx < bufsiz
src/hotspot/os/windows/memMapPrinter_windows.cpp line 114:
> 112: return buffer;
> 113: }
> 114: const char* getStateString(char* buffer, size_t bufsiz,
> MEMORY_BASIC_INFORMATION& mInfo) {
We have a utility class for printing to a limited buffer (stringStream). It
takes care of buffer limits and zero termination for you. I would use that one
where possible.
stringStream ss(buffer, bufsiz);
if (mInfo.State == MEM_COMMIT) {
ss.put('c');
}
...
else { ss.print("0x%x", mInfo.State); }
return buffer;
src/hotspot/os/windows/memMapPrinter_windows.cpp line 145:
> 143: size_t _total_region_size; // combined resident set size
> 144: size_t _total_committed;// combined committed size
> 145: size_t _total_reserved; // combined shared size
I would like us to be careful with terminology since that can get confusing
quickly when jumping between OS code and generic code.
In hotspot terminology, "reserved" are parts of the address space that have
been allocated for the process, *regardless of commit state*. So, the whole
region would count as "reserved", with some pages being also committed, and
some not. But on Windows, the MEM_RESERVE state is only given to uncommitted
regions, so there is a subtle difference.
I would circumvent the problem by counting only:
- total size (as in sum of region sizes that are not MEM_FREE)
- committed size (as in sum of region sizes that are MEM_COMMIT)
src/hotspot/os/windows/memMapPrinter_windows.cpp line 162:
> 160: outputStream* st = session.out();
> 161: os::print_os_info(st);
> 162: os::print_memory_info(st);
The "danger" here is that both of these functions may bloat in the future and
print a lot more than today; and that we may want to use this printout in
places where we also print out OS info and memory info, and therefore get
duplicated printout (e.g. hs-err printing or jcmd VM.info).
But I would print out the number of mappings you counted, and the total size of
committed regions.
src/hotspot/os/windows/memMapPrinter_windows.cpp line 243:
> 241: printer.print_header();
> 242:
> 243: MEMORY_BASIC_INFORMATION mInfo;
Just to be sure, I would zero out mInfo.
src/hotspot/os/windows/memMapPrinter_windows.cpp line 246:
> 244: MappingInfo info;
> 245:
> 246: for (char* ptr = 0; VirtualQueryEx(hProcess, ptr, &mInfo,
> sizeof(mInfo)) == sizeof(mInfo); ptr += mInfo.RegionSize) {
I would add a failsafe breakout in case we made a thinking error somewhere or
the address space is insanely fragmented. Maybe break out after a million
regions? Or, after a certain time (e.g. 10 seconds). I also would assert for
RegionSize > 0
src/hotspot/share/services/diagnosticCommand.cpp line 1207:
> 1205: const char* absname = os::Posix::realpath(name, t
Re: RFR: 8319873: Add windows implementation for jcmd System.map and System.dump_map
On Tue, 20 Aug 2024 06:36:26 GMT, Thomas Stuefe wrote:
>> This is a port of [JDK-8318636](https://github.com/openjdk/jdk/pull/16301)
>> to Windows.
>>
>> System.map and System.dump_map are implemented using the Windows API and
>> provide roughly the same information in the same format. Most of the heavy
>> lifting was implemented by @tstuefe in #16301 - this PR adds the Windows
>> implementation and enables the common code for Windows 64 bit.
>>
>> [Sample output (with NMT
>> enabled)](https://github.com/user-attachments/files/16663332/vm_memory_map_760.txt)
>
> src/hotspot/os/windows/memMapPrinter_windows.cpp line 98:
>
>> 96: buffer[idx++] = 'n';
>> 97: } else if (prot != 0) {
>> 98: snprintf(buffer, bufsiz, "(0x%x)", prot);
>
> This could truncate for the longest possible value (16+4 chars).
But (here and in other places) raw-printing the unknown constant may not be the
best way. Chances are this code is executed rarely and most people will just be
perplexed at the weird printouts.
We could assert here: After all, we expect to handle all possible values; if we
see an unknown constant, then we either have a bug in the code and what we read
here is no protection value, or Microsoft added new constants. In both cases, a
code fix is needed. Asserts would fire up in automatic regression tests.
The alternative is to print an '?'. And possibly scan for that in regression
tests too.
I leave this decision up to you.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722805594
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v2]
On Wed, 13 Sep 2023 17:38:28 GMT, Justin Lu wrote: >> JDK .properties files still use ISO-8859-1 encoding with escape sequences. >> It would improve readability to see the native characters instead of escape >> sequences (especially for the L10n process). The majority of files changed >> are localized resource files. >> >> This change converts the Unicode escape sequences in the JDK .properties >> files (both in src and test) to UTF-8 native characters. Additionally, the >> build logic is adjusted to read the .properties files in UTF-8 while >> generating the ListResourceBundle files. >> >> The only escape sequence not converted was `\u0020` as this is used to >> denote intentional trailing white space. (E.g. `key=This is the >> value:\u0020`) >> >> The conversion was done using native2ascii with options `-reverse -encoding >> UTF-8`. >> >> If this PR is integrated, the IDE default encoding for .properties files >> need to be updated to UTF-8. (IntelliJ IDEA locks .properties files as >> ISO-8859-1 unless manually changed). > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Replace InputStreamReader with BufferedReader src/jdk.jartool/share/classes/sun/tools/jar/resources/jar_pt_BR.properties line 95: > 93: > 94: main.usage.summary=Uso: jar [OPTION...] [ [--release VERSION] [-C dir] > files] ... > 95: main.usage.summary.try=Tente `jar --ajuda' para obter mais informações. I was looking for something unrelated in properties files, and found this. It is surprising to see an option name being localised; it must be a bug. - PR Review Comment: https://git.openjdk.org/jdk/pull/15694#discussion_r1722966688
Re: RFR: 8338482: com/sun/jdi/ThreadMemoryLeakTest.java requires that compressed oops are enabled
On Fri, 16 Aug 2024 05:41:01 GMT, Chris Plummer wrote: > com/sun/jdi/ThreadMemoryLeakTest.java purposely runs with a small heap so it > is easier to detect a memory leak with an OOME. Running without compressed > oops uses more memory leading to an OOME even though there is no leak. We > should require that this test be run only with compressed oops. > > Tested by running just this test without compress oops and verifying that the > test is not run, and also running with compressed oops and verifying that it > is run. I also did an `-Xcomp` run to verify that it still is not run with > `-Xcomp`. Looks good. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20606#pullrequestreview-2247436712
[jdk23] RFR: 8338139: {ClassLoading, Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
Hi all, This pull request contains a backport of commit [9775d571](https://github.com/openjdk/jdk/commit/9775d57168695dc0d758e017fe5069d93d593f3e) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Stefan Karlsson on 20 Aug 2024 and was reviewed by Leonid Mesnik, Daniel D. Daugherty and David Holmes. Thanks! - Commit messages: - Backport 9775d57168695dc0d758e017fe5069d93d593f3e Changes: https://git.openjdk.org/jdk/pull/20643/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20643&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8338139 Stats: 202 lines in 6 files changed: 196 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/20643.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20643/head:pull/20643 PR: https://git.openjdk.org/jdk/pull/20643
Re: [jdk23] RFR: 8338139: {ClassLoading,Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
On Tue, 20 Aug 2024 12:47:59 GMT, Stefan Karlsson wrote: > Hi all, > > This pull request contains a backport of commit > [9775d571](https://github.com/openjdk/jdk/commit/9775d57168695dc0d758e017fe5069d93d593f3e) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Stefan Karlsson on 20 Aug 2024 > and was reviewed by Leonid Mesnik, Daniel D. Daugherty and David Holmes. > > Thanks! Backport looks good. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20643#pullrequestreview-2247954598
8315884: New Object to ObjectMonitor mapping causes linux-aarch64 musl to fail
Hi all
Recently we at JetBrains became getting the following build failure for Linux
aarch64 musl
===8<---
. . .
Compiling up to 55 files for jdk.jpackage
/mnt/agent/work/f25b6e4d8156543c/src/hotspot/share/runtime/synchronizer.cpp: In
static member function 'static intptr_t
ObjectSynchronizer::FastHashCode(Thread*, oop)':
/mnt/agent/work/f25b6e4d8156543c/src/hotspot/share/runtime/synchronizer.cpp:1116:1:
error: unable to generate reloads for:
1116 | }
| ^
(insn 565 564 566 28 (set (reg/v:DI 2 x2 [ reg2 ])
(ior:DI (and:DI (ashift:DI (reg/v:DI 188 [ ])
(const_int 8 [0x8]))
(const_int 549755813632 [0x7fff00]))
(and:DI (reg/v:DI 1 x1 [ reg1 ])
(const_int -549755813633 [0xff8000ff]
"/mnt/agent/work/f25b6e4d8156543c/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp":49:21
792 {*aarch64_bfidi5_shift_alt}
(nil))
during RTL pass: reload
/mnt/agent/work/f25b6e4d8156543c/src/hotspot/share/runtime/synchronizer.cpp:1116:1:
internal compiler error: in curr_insn_transform, at lra-constraints.c:3962
Please submit a full bug report,
with preprocessed source if appropriate.
See
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v2]
On Tue, 20 Aug 2024 09:07:54 GMT, Pavel Rappo wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Replace InputStreamReader with BufferedReader > > src/jdk.jartool/share/classes/sun/tools/jar/resources/jar_pt_BR.properties > line 95: > >> 93: >> 94: main.usage.summary=Uso: jar [OPTION...] [ [--release VERSION] [-C dir] >> files] ... >> 95: main.usage.summary.try=Tente `jar --ajuda' para obter mais informações. > > I was looking for something unrelated in properties files, and found this. It > is surprising to see an option name being localised; it must be a bug. Good catch, Pavel. It is indeed a bug. This type of overtranslation l10n bug happens all the time, and hard to catch. - PR Review Comment: https://git.openjdk.org/jdk/pull/15694#discussion_r1723520963
Re: RFR: 8338139: {ClassLoading, Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods [v2]
On Tue, 20 Aug 2024 06:13:18 GMT, Stefan Karlsson wrote: >> So in this test case: >> `-Xlog:gc,gc+init` >> you have it so that `true` is returned. >> >> With this line of code: >> ``` >>const bool is_gc_exact_match = ts->contains(LogTag::_gc) && ts->ntags() >> == 1; >> >> I would think that this part is `false`: `ts->ntags() == 1` >> for the `-Xlog:gc,gc+init` test case so we'll be returning `false`. >> >> What am I missing? > > The important part is that `ts` contains one single tag set and that > `gc,gc+init` is a string that specifies the two tag sets `gc` and `gc+init`. > Also, note that `-Xlog:gc,gc+init` and `-Xlog:gc -Xlog:gc+init` is > effectively the same. > > It is also important to understand that the for loop iterates over all tag > sets that have been used in the HotSpot code. It is not iterating over the > tag sets specified by -Xlog lines. OK, I get it now. The loop cycles thru the tag set and finds one that is a singleton and an exact match, i.e., `-Xlog:gc`. With `-Xlog:gc+init`, the `ts->ntags() == 1` would be `false` because that one is not an exact match. I misunderstood the algorithm a bit. Sorry for the noise. - PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1723555244
Re: [jdk23] RFR: 8338139: {ClassLoading,Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
On Tue, 20 Aug 2024 12:47:59 GMT, Stefan Karlsson wrote: > Hi all, > > This pull request contains a backport of commit > [9775d571](https://github.com/openjdk/jdk/commit/9775d57168695dc0d758e017fe5069d93d593f3e) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Stefan Karlsson on 20 Aug 2024 > and was reviewed by Leonid Mesnik, Daniel D. Daugherty and David Holmes. > > Thanks! Thumbs up. Re-reviewed via GitHub and compared the JDK24 patch file with the JDK23 patch file. - PR Review: https://git.openjdk.org/jdk/pull/20643#pullrequestreview-2248424096 PR Comment: https://git.openjdk.org/jdk/pull/20643#issuecomment-2299210408
Re: [jdk23] RFR: 8338139: {ClassLoading,Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
On Tue, 20 Aug 2024 12:47:59 GMT, Stefan Karlsson wrote: > Hi all, > > This pull request contains a backport of commit > [9775d571](https://github.com/openjdk/jdk/commit/9775d57168695dc0d758e017fe5069d93d593f3e) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Stefan Karlsson on 20 Aug 2024 > and was reviewed by Leonid Mesnik, Daniel D. Daugherty and David Holmes. > > Thanks! Thanks for the review - PR Comment: https://git.openjdk.org/jdk/pull/20643#issuecomment-2299195665
[jdk23] Integrated: 8338139: {ClassLoading,Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
On Tue, 20 Aug 2024 12:47:59 GMT, Stefan Karlsson wrote:
> Hi all,
>
> This pull request contains a backport of commit
> [9775d571](https://github.com/openjdk/jdk/commit/9775d57168695dc0d758e017fe5069d93d593f3e)
> from the [openjdk/jdk](https://git.openjdk.org/jdk) repository.
>
> The commit being backported was authored by Stefan Karlsson on 20 Aug 2024
> and was reviewed by Leonid Mesnik, Daniel D. Daugherty and David Holmes.
>
> Thanks!
This pull request has now been integrated.
Changeset: 9ad2e63f
Author:Stefan Karlsson
URL:
https://git.openjdk.org/jdk/commit/9ad2e63f176364b96a827af80055e7db4b61fc9a
Stats: 202 lines in 6 files changed: 196 ins; 2 del; 4 mod
8338139: {ClassLoading,Memory}MXBean::isVerbose methods are inconsistent with
their setVerbose methods
Reviewed-by: kevinw
Backport-of: 9775d57168695dc0d758e017fe5069d93d593f3e
-
PR: https://git.openjdk.org/jdk/pull/20643
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Thu, 15 Aug 2024 20:28:28 GMT, Dhamoder Nalla wrote: >> Use the GetTempPath2 APIs instead of the GetTempPath APIs in native code >> across the OpenJDK repository to retrieve the temporary directory path, as >> GetTempPath2 provides enhanced security. While GetTempPath may still >> function without errors, using GetTempPath2 reduces the risk of potential >> exploits for users. >> >> >> The code to dynamically load GetTempPath2 is duplicated due to the following >> reasons. I would appreciate any suggestions to remove the duplication where >> possible: >> >> 1. The changes span across four different folders—java.base, jdk.package, >> jdk.attach, and hotspot—with no shared code between them. >> 2. Some parts of the code use version A, while others use version W (ANSI >> vs. Unicode). >> 3. Some parts of the code are written in C others in C++. > > Dhamoder Nalla has updated the pull request incrementally with one additional > commit since the last revision: > > fix missing code src/java.base/windows/native/libjava/java_props_md.c line 327: > 325: typedef DWORD (WINAPI *GetTempPath2WFnPtr)(DWORD, LPWSTR); > 326: static GetTempPath2WFnPtr _GetTempPath2W = NULL; > 327: static BOOL _GetTempPath2WInitialized = FALSE; GetJavaProperties should only be used once so I don't think you need to cache it. Also I'm wondering if we can link to the function rather than using GetProcAddress. It looks like GetTempPath2 was added in Windows 8 + Windows Server 2012. I wonder if there is anyone building main line to older SDKs or Windows releases where linking to GetTempPath2 would fail. - PR Review Comment: https://git.openjdk.org/jdk/pull/20600#discussion_r1723600551
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value
On Wed, 14 Aug 2024 19:34:26 GMT, Leonid Mesnik wrote: > The summary of the problem. Test is intermittently failing because can't get > expected field watch event. > The test is failing to get event in the 'setfmodw001b' thread with Xcomp only. > I verified that frame with the method 'run' of setfmodw001b is compiled when > line > 118: setfmodw001.setWatch(4); > is executed, however the thread is in interp_only mode. The watch events are > supported by interpreter only and just ignored by compiled code. > > The reason of failure is race beteween setting interp_only mode in line > > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 > > and calling method call_helper while > the method run() > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 > > in newly created thread 'setfmodw001b' is invoked. > > The javaCalls:call are used to invoke methods from hotspot, so it might be > rare issues. But still, synchronization might be improved. > The > void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, > JavaCallArguments* args, TRAPS) > > checks if interp_only mode is set and use 'Method::from_interpreted_entry()' > if not. However the interp_only might be set later before compiled method is > called (or enter first safe point?). This might happens in safepoint during > transition via handshake. > So the running thread is in interp_only mode however the run() method is > compiled and executed already and never going to be deoptimized. > > The additional setWatch calls don't try to deptimize method that are already > in interp_only mode. > > BTW, the when JVMCI is enabled and verified adapter exists it is also will be > loaded even in interp_only mode set. There is not race here, just ignoring of > interp_only mode. > > I run failing test with Xcomp ~1000 times and tiers1-5. I was able to reproduce failure in "-Xcomp +ZGC'' once in a couple of several hundred runs. Not reproduced anymore with thousands of executions. Run tier1-5 to ensure all svc tests passed. There is no easy way to develop regression test. It should provoke call_helper for compiled method with breakpoint and setting the only single setWatch. Not sure it can find anything in reasonable time. The call_helper is used only to call methods from hotspot directly, like , run() for Thread.start and similar not very common methods. I think to review any other possible cases and and thread stack verification that check that interp_only thread don't contain compiled frames on the stack. So we could find similar issues using our testing. Need to find the good place to inject this self-check. There are also a couple of places for improvements in this events handling. They are not directly rtelated to this bug. Will file them separately. - PR Comment: https://git.openjdk.org/jdk/pull/20587#issuecomment-2299317622
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value
On Wed, 14 Aug 2024 19:34:26 GMT, Leonid Mesnik wrote:
> The summary of the problem. Test is intermittently failing because can't get
> expected field watch event.
> The test is failing to get event in the 'setfmodw001b' thread with Xcomp only.
> I verified that frame with the method 'run' of setfmodw001b is compiled when
> line
> 118: setfmodw001.setWatch(4);
> is executed, however the thread is in interp_only mode. The watch events are
> supported by interpreter only and just ignored by compiled code.
>
> The reason of failure is race beteween setting interp_only mode in line
>
> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75
>
> and calling method call_helper while
> the method run()
> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116
>
> in newly created thread 'setfmodw001b' is invoked.
>
> The javaCalls:call are used to invoke methods from hotspot, so it might be
> rare issues. But still, synchronization might be improved.
> The
> void JavaCalls::call_helper(JavaValue* result, const methodHandle& method,
> JavaCallArguments* args, TRAPS)
>
> checks if interp_only mode is set and use 'Method::from_interpreted_entry()'
> if not. However the interp_only might be set later before compiled method is
> called (or enter first safe point?). This might happens in safepoint during
> transition via handshake.
> So the running thread is in interp_only mode however the run() method is
> compiled and executed already and never going to be deoptimized.
>
> The additional setWatch calls don't try to deptimize method that are already
> in interp_only mode.
>
> BTW, the when JVMCI is enabled and verified adapter exists it is also will be
> loaded even in interp_only mode set. There is not race here, just ignoring of
> interp_only mode.
>
> I run failing test with Xcomp ~1000 times and tiers1-5.
src/hotspot/share/runtime/javaCalls.cpp line 403:
> 401: } else {
> 402: // Since the call stub sets up like the interpreter we call
> the from_interpreted_entry
> 403: // so we can go compiled via a i2c.
Do you think removed comment "Otherwise initial entry method will always run
interpreted.", or it's understood and redundant?
src/hotspot/share/runtime/javaCalls.cpp line 413:
> 411: // respect to nmethod sweeping.
> 412: address
> 413: verified_entry_point = (address)
> HotSpotJVMCI::InstalledCode::entryPoint(nullptr, alternative_target());
This line-break choice seems unconventional. Can we just make it a single line
like before?
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001/TestDescription.java
line 57:
> 55: * /test/lib
> 56: * @run main/othervm/native -agentlib:setfmodw001
> -XX:TraceJVMTI=ec+,+ioe,+s -Xlog:jvmti=trace:file=vm.%p.log
> nsk.jvmti.SetFieldModificationWatch.setfmodw001
> 57: */
What's the reason for removing this test run?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723692429
PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723691524
PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723690592
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value [v2]
> The summary of the problem. Test is intermittently failing because can't get > expected field watch event. > The test is failing to get event in the 'setfmodw001b' thread with Xcomp only. > I verified that frame with the method 'run' of setfmodw001b is compiled when > line > 118: setfmodw001.setWatch(4); > is executed, however the thread is in interp_only mode. The watch events are > supported by interpreter only and just ignored by compiled code. > > The reason of failure is race beteween setting interp_only mode in line > > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 > > and calling method call_helper while > the method run() > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 > > in newly created thread 'setfmodw001b' is invoked. > > The javaCalls:call are used to invoke methods from hotspot, so it might be > rare issues. But still, synchronization might be improved. > The > void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, > JavaCallArguments* args, TRAPS) > > checks if interp_only mode is set and use 'Method::from_interpreted_entry()' > if not. However the interp_only might be set later before compiled method is > called (or enter first safe point?). This might happens in safepoint during > transition via handshake. > So the running thread is in interp_only mode however the run() method is > compiled and executed already and never going to be deoptimized. > > The additional setWatch calls don't try to deptimize method that are already > in interp_only mode. > > BTW, the when JVMCI is enabled and verified adapter exists it is also will be > loaded even in interp_only mode set. There is not race here, just ignoring of > interp_only mode. > > I run failing test with Xcomp ~1000 times and tiers1-5. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fixed identation. - Changes: - all: https://git.openjdk.org/jdk/pull/20587/files - new: https://git.openjdk.org/jdk/pull/20587/files/fdd9f91c..2a148a29 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20587&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20587&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20587.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20587/head:pull/20587 PR: https://git.openjdk.org/jdk/pull/20587
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value [v2]
On Tue, 20 Aug 2024 17:28:12 GMT, Dean Long wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed identation.
>
> src/hotspot/share/runtime/javaCalls.cpp line 403:
>
>> 401: } else {
>> 402: // Since the call stub sets up like the interpreter we call
>> the from_interpreted_entry
>> 403: // so we can go compiled via a i2c.
>
> Do you think removed comment "Otherwise initial entry method will always run
> interpreted.", or it's understood and redundant?
Yes. I believe that expression in the 'if' clearly explain the reason of
using interpreter. So comment is redundant.
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001/TestDescription.java
> line 57:
>
>> 55: * /test/lib
>> 56: * @run main/othervm/native -agentlib:setfmodw001
>> -XX:TraceJVMTI=ec+,+ioe,+s -Xlog:jvmti=trace:file=vm.%p.log
>> nsk.jvmti.SetFieldModificationWatch.setfmodw001
>> 57: */
>
> What's the reason for removing this test run?
I was not able to reproduce failure locally at the beginning. So I added this
test run just to get more information about failure from CI runs for initial
analysis.
The information from logging clearly pointed that we just never got event while
set Thread-1 to interprter_only mode correctly.
However, it is not needed now so I am removing it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723701025
PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723704840
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value [v2]
On Tue, 20 Aug 2024 17:38:01 GMT, Leonid Mesnik wrote: >> The summary of the problem. Test is intermittently failing because can't get >> expected field watch event. >> The test is failing to get event in the 'setfmodw001b' thread with Xcomp >> only. >> I verified that frame with the method 'run' of setfmodw001b is compiled when >> line >> 118: setfmodw001.setWatch(4); >> is executed, however the thread is in interp_only mode. The watch events are >> supported by interpreter only and just ignored by compiled code. >> >> The reason of failure is race beteween setting interp_only mode in line >> >> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 >> >> and calling method call_helper while >> the method run() >> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 >> >> in newly created thread 'setfmodw001b' is invoked. >> >> The javaCalls:call are used to invoke methods from hotspot, so it might be >> rare issues. But still, synchronization might be improved. >> The >> void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, >> JavaCallArguments* args, TRAPS) >> >> checks if interp_only mode is set and use 'Method::from_interpreted_entry()' >> if not. However the interp_only might be set later before compiled method is >> called (or enter first safe point?). This might happens in safepoint during >> transition via handshake. >> So the running thread is in interp_only mode however the run() method is >> compiled and executed already and never going to be deoptimized. >> >> The additional setWatch calls don't try to deptimize method that are already >> in interp_only mode. >> >> BTW, the when JVMCI is enabled and verified adapter exists it is also will >> be loaded even in interp_only mode set. There is not race here, just >> ignoring of interp_only mode. >> >> I run failing test with Xcomp ~1000 times and tiers1-5. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > fixed identation. Looks good. - Marked as reviewed by dlong (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20587#pullrequestreview-2248663420
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value [v2]
On Tue, 20 Aug 2024 18:38:49 GMT, Serguei Spitsyn wrote: >> Leonid Mesnik has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fixed identation. > > Marked as reviewed by sspitsyn (Reviewer). @sspitsyn, @dean-long Thank you for review - PR Comment: https://git.openjdk.org/jdk/pull/20587#issuecomment-2299508676
Integrated: 8205957: setfldw001/TestDescription.java fails with bad field value
On Wed, 14 Aug 2024 19:34:26 GMT, Leonid Mesnik wrote: > The summary of the problem. Test is intermittently failing because can't get > expected field watch event. > The test is failing to get event in the 'setfmodw001b' thread with Xcomp only. > I verified that frame with the method 'run' of setfmodw001b is compiled when > line > 118: setfmodw001.setWatch(4); > is executed, however the thread is in interp_only mode. The watch events are > supported by interpreter only and just ignored by compiled code. > > The reason of failure is race beteween setting interp_only mode in line > > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 > > and calling method call_helper while > the method run() > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 > > in newly created thread 'setfmodw001b' is invoked. > > The javaCalls:call are used to invoke methods from hotspot, so it might be > rare issues. But still, synchronization might be improved. > The > void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, > JavaCallArguments* args, TRAPS) > > checks if interp_only mode is set and use 'Method::from_interpreted_entry()' > if not. However the interp_only might be set later before compiled method is > called (or enter first safe point?). This might happens in safepoint during > transition via handshake. > So the running thread is in interp_only mode however the run() method is > compiled and executed already and never going to be deoptimized. > > The additional setWatch calls don't try to deptimize method that are already > in interp_only mode. > > BTW, the when JVMCI is enabled and verified adapter exists it is also will be > loaded even in interp_only mode set. There is not race here, just ignoring of > interp_only mode. > > I run failing test with Xcomp ~1000 times and tiers1-5. This pull request has now been integrated. Changeset: c646efc3 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/c646efc366342564baebd2f17133e14780abcaa8 Stats: 48 lines in 3 files changed: 15 ins; 22 del; 11 mod 8205957: setfldw001/TestDescription.java fails with bad field value Reviewed-by: sspitsyn, dlong - PR: https://git.openjdk.org/jdk/pull/20587
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value [v2]
On Tue, 20 Aug 2024 17:38:01 GMT, Leonid Mesnik wrote: >> The summary of the problem. Test is intermittently failing because can't get >> expected field watch event. >> The test is failing to get event in the 'setfmodw001b' thread with Xcomp >> only. >> I verified that frame with the method 'run' of setfmodw001b is compiled when >> line >> 118: setfmodw001.setWatch(4); >> is executed, however the thread is in interp_only mode. The watch events are >> supported by interpreter only and just ignored by compiled code. >> >> The reason of failure is race beteween setting interp_only mode in line >> >> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 >> >> and calling method call_helper while >> the method run() >> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 >> >> in newly created thread 'setfmodw001b' is invoked. >> >> The javaCalls:call are used to invoke methods from hotspot, so it might be >> rare issues. But still, synchronization might be improved. >> The >> void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, >> JavaCallArguments* args, TRAPS) >> >> checks if interp_only mode is set and use 'Method::from_interpreted_entry()' >> if not. However the interp_only might be set later before compiled method is >> called (or enter first safe point?). This might happens in safepoint during >> transition via handshake. >> So the running thread is in interp_only mode however the run() method is >> compiled and executed already and never going to be deoptimized. >> >> The additional setWatch calls don't try to deptimize method that are already >> in interp_only mode. >> >> BTW, the when JVMCI is enabled and verified adapter exists it is also will >> be loaded even in interp_only mode set. There is not race here, just >> ignoring of interp_only mode. >> >> I run failing test with Xcomp ~1000 times and tiers1-5. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > fixed identation. Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20587#pullrequestreview-2248773043
Re: RFR: 8337517: Redacted Heap Dumps [v2]
On Thu, 1 Aug 2024 21:46:04 GMT, Henry Lin wrote: >> Adds a command line option `-redact` to `jcmd` and `-XX:+HeapDumpRedacted` >> enabling redacted heap dumps. When enabled, the output binary heap dump has >> zeroes written out in place of the original primitive values in the object >> fields. There is a new jtreg test `heapDumpRedactedTest.java` that tests >> that the fields are properly redacted. > > Henry Lin has updated the pull request incrementally with three additional > commits since the last revision: > > - refactored redact conditional > - format HeapDumpRedactedTest.java > - Revert "add jmap redact" > >This reverts commit e87b74e7d9d58867f7d64bb0ae331cba665e05d4. This review has gone quiet. I sense that there are no major technical objections to the change, but that the consensus prefers to leave this functionality to external tools. If there are no further comments by the end of the week, we will withdraw this pull request. - PR Comment: https://git.openjdk.org/jdk/pull/20409#issuecomment-2299665864
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Tue, 20 Aug 2024 16:17:24 GMT, Alan Bateman wrote: >> Dhamoder Nalla has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fix missing code > > src/java.base/windows/native/libjava/java_props_md.c line 327: > >> 325: typedef DWORD (WINAPI *GetTempPath2WFnPtr)(DWORD, LPWSTR); >> 326: static GetTempPath2WFnPtr _GetTempPath2W = NULL; >> 327: static BOOL _GetTempPath2WInitialized = FALSE; > > GetJavaProperties should only be used once so I don't think you need to cache > it. > > Also I'm wondering if we can link to the function rather than using > GetProcAddress. It looks like GetTempPath2 was added in Windows 8 + Windows > Server 2012. I wonder if there is anyone building main line to older SDKs or > Windows releases where linking to GetTempPath2 would fail. Thanks @AlanBateman for reviewing this PR. GetTempPath2 is available in Windows10 Build 20348 and above as per https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppath2w#requirements - PR Review Comment: https://git.openjdk.org/jdk/pull/20600#discussion_r1723963431
Re: RFR: 8338257: UTF8 lengths should be size_t not int
On Thu, 15 Aug 2024 20:44:52 GMT, Coleen Phillimore wrote: >> This work has been split out from JDK-8328877: [JNI] The JNI Specification >> needs to address the limitations of integer UTF-8 String lengths >> >> The modified UTF-8 format used by the VM can require up to six bytes to >> represent one unicode character, but six byte characters are stored as >> UTF-16 surrogate pairs. Hence the most bytes per character is 3, and so the >> maximum length is 3*`Integer.MAX_VALUE`. Though with compact strings this >> reduces to 2*`Integer.MAX_VALUE`. The low-level UTF8/UNICODE API should >> therefore define UTF8 lengths as `size_t` to accommodate all possible >> representations. Higher-level API's can still use `int` if they know the >> strings (eg symbols) are sufficiently constrained in length. See the >> comments in utf8.hpp that explain Strings, compact strings and the encoding. >> >> As the existing JNI `GetStringUTFLength` still requires the current >> truncating behaviour of ` UNICODE::utf8_length` we add back >> `UNICODE::utf8_length_as_int` for it to use. >> >> Note that some API's, like ` UNICODE::as_utf8(const T* base, size_t& >> length)` use `length` as an IN/OUT parameter: it is the incoming (int) >> length of the jbyte/jchar array, and the outgoing (size_t) length of the >> UTF8 sequence. This makes some of the call sites a little messy with casts. >> >> Testing: >> - tiers 1-4 >> - GHA > > It doesn't look like GHA is configured for you here. @coleenp I compiled with `-Wconversion` on Linux and there were no warnings in relation to the code I have changed. - PR Comment: https://git.openjdk.org/jdk/pull/20560#issuecomment-2299884096
Re: RFR: 8338482: com/sun/jdi/ThreadMemoryLeakTest.java requires that compressed oops are enabled
On Fri, 16 Aug 2024 05:41:01 GMT, Chris Plummer wrote: > com/sun/jdi/ThreadMemoryLeakTest.java purposely runs with a small heap so it > is easier to detect a memory leak with an OOME. Running without compressed > oops uses more memory leading to an OOME even though there is no leak. We > should require that this test be run only with compressed oops. > > Tested by running just this test without compress oops and verifying that the > test is not run, and also running with compressed oops and verifying that it > is run. I also did an `-Xcomp` run to verify that it still is not run with > `-Xcomp`. Thanks you for the reviews Alex and Kevin! - PR Comment: https://git.openjdk.org/jdk/pull/20606#issuecomment-2299929041
Integrated: 8338482: com/sun/jdi/ThreadMemoryLeakTest.java requires that compressed oops are enabled
On Fri, 16 Aug 2024 05:41:01 GMT, Chris Plummer wrote: > com/sun/jdi/ThreadMemoryLeakTest.java purposely runs with a small heap so it > is easier to detect a memory leak with an OOME. Running without compressed > oops uses more memory leading to an OOME even though there is no leak. We > should require that this test be run only with compressed oops. > > Tested by running just this test without compress oops and verifying that the > test is not run, and also running with compressed oops and verifying that it > is run. I also did an `-Xcomp` run to verify that it still is not run with > `-Xcomp`. This pull request has now been integrated. Changeset: d7281079 Author:Chris Plummer URL: https://git.openjdk.org/jdk/commit/d72810794bf70f82e46f7220698e4d27d5973d5b Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod 8338482: com/sun/jdi/ThreadMemoryLeakTest.java requires that compressed oops are enabled Reviewed-by: amenkov, kevinw - PR: https://git.openjdk.org/jdk/pull/20606
