Re: RFR: 8231585: java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java fails with java.lang.NullPointerException
Hi Daniil, LGTM++ Thanks, Serguei On 4/17/20 14:14, Chris Plummer wrote: Looks good. Chris On 4/17/20 1:03 PM, Daniil Titov wrote: Please review the change that fixes intermittent failure of java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java As David noticed (thank you, David, for this analysis) there is no guarantee that all threads found by getAllThreadIds() are still alive by the time we call getThreadInfo() so we have to allow for null array entries. Testing: Mach5 tests with Graal on passed 300 times. [1] http://cr.openjdk.java.net/~dtitov/8231585/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8231585 Best regards, Daniil
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
On 4/17/20 16:52, Mandy Chung wrote: On 4/17/20 3:51 PM, Chris Plummer wrote: Hi Mandy, Thanks for updating the svc specs. Some comments below: In the JDWP spec update, you changed "JNI signature" to "type signature" in one place, but left it as "JNI signature" everywhere else. Should they all be changed? JDWP signature is changed because there is no JNI signature representing a hidden class. I leave other references to JNI signature as is since those only apply for normal classes as I wanted to keep this change specifically due to hidden classes. I think it's good to file a JBS issue to follow up on JDWP and JDI to determine if the spec should be upgraded to reference the new TypeDescriptor API. In the JDWP spec for ClassLoaderReference.VisibleClasses: "That is, this class loader has been recorded as an initiating loader of the returned classes." -> "That is, all classes for which this class loader has been recorded as an initiating loader." This seems like too much detail to be put here. Basically the term "initiating ClassLoader" has turned into a short essay. Is it possible that all this detail could be put elsewhere and referenced? Any suggestion? We attempted to place those description in JVM TI Class section or ClassLoad event. However, that's not ideal place since that's needed by JDWP, JDI and Instrumentation. I found inlining this description is not ideal but it provides adequate clarification. The JDI (transitively via JDWP), JDWP and Instrumentation implementations are based on the JVMTI. I've tried to suggest once to link these API's to the JVMTI. The problem is there was no such practice in the specs of these API's before but we can make a step to introduce it now. Placing this description either in JVM TI Class section or ClassLoad event would be good enough. An alternative approach is to make JVMTI/JDI/JDWP/Instrument to refer to the java.lang.Class spec for general information about class loading and classes defined and initiated by class loaders. Thanks, Serguei Aren't there other places in other specs where a similar clarification of "initiating ClassLoader" is needed (I see now that ClassLoaderClasses in the JVMTI spec, ClassLoaderReference,visibleClasses in the JDI spec, and Instrumentation.getInitiatedClasses are all dealing with this, but not all in the exact same way). I took the conservative side and make sure the clarification is in place for all APIs. I'm open to any suggestion for example having JDWP and JDI to link to JVM TI spec if you think appropriate. In the JVMTI spec for GetLoadedClasses: This suffers in a way similar to ClassLoaderReference.VisibleClasses in the JDWP spec, although not as badly. A simple concept ends up with a complex description, and it seems that description should really be in a more centralized place. I would also suggest a bit of cleanup of these lines: 6866 An array class is created directly by Java virtual machine. An array class 6867 creation can be triggered by using class loaders or by invoking methods in certain 6868 Java SE Platform API such as reflection. "Created by [the] Java virtual machine" (add "the") Change "An array class creation" to "The creation" since your are repeating "An array class" from the previous sentence. In the JVMTI spec ClassLoaderClasses section: "That is, initiating_loader has been recorded as an initiating loader of the returned classes." -> "That is, all classes for which initiating_loader has been recorded as an initiating loader." In the JVMTI spec GetClassSignature section: "If the class indicated by klass is ..." -> "If the class ..." (you just finished the previous sentence with "class indicated by Klass"). "the returned name is [the] JNI type signature" (add "the"). Also, is "JNI type signature" formally defined somewhere? This relates to my JDWP spec comment above. It's a link to https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. This is how the current JVM TI spec defines. " where N is the binary name encoded in internal form indicated by the class file". Is "binary name encoded in internal form" explained somewhere? JVMS 4.2.1 https://docs.oracle.com/javase/specs/jvms/se14/html/jvms-4.html#jvms-4.2.1 Also, can you add an example of a returned hidden class signature? OK In the JVMTI spec ClassLoad section: "representation using [a] class loader" (add "a") "By invoking Lookup::defineHiddenClass, that creates ..." -> "By invoking Lookup::defineHiddenClass to create ..." "certain Java SE Platform API" -> Should be "APIs" In JDI ClassLoaderReference.definedClasses() "loaded at least to the point of preparation and types ..." -> "loaded at least to the point of preparation, and types ..." (Note, this not a new issue with your edits) In Instrumentation.getAllLoadedClasses: The reference to `class` did
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
On 4/17/20 3:51 PM, Chris Plummer wrote: Hi Mandy, Thanks for updating the svc specs. Some comments below: In the JDWP spec update, you changed "JNI signature" to "type signature" in one place, but left it as "JNI signature" everywhere else. Should they all be changed? JDWP signature is changed because there is no JNI signature representing a hidden class. I leave other references to JNI signature as is since those only apply for normal classes as I wanted to keep this change specifically due to hidden classes. I think it's good to file a JBS issue to follow up on JDWP and JDI to determine if the spec should be upgraded to reference the new TypeDescriptor API. In the JDWP spec for ClassLoaderReference.VisibleClasses: "That is, this class loader has been recorded as an initiating loader of the returned classes." -> "That is, all classes for which this class loader has been recorded as an initiating loader." This seems like too much detail to be put here. Basically the term "initiating ClassLoader" has turned into a short essay. Is it possible that all this detail could be put elsewhere and referenced? Any suggestion? We attempted to place those description in JVM TI Class section or ClassLoad event. However, that's not ideal place since that's needed by JDWP, JDI and Instrumentation. I found inlining this description is not ideal but it provides adequate clarification. Aren't there other places in other specs where a similar clarification of "initiating ClassLoader" is needed (I see now that ClassLoaderClasses in the JVMTI spec, ClassLoaderReference,visibleClasses in the JDI spec, and Instrumentation.getInitiatedClasses are all dealing with this, but not all in the exact same way). I took the conservative side and make sure the clarification is in place for all APIs. I'm open to any suggestion for example having JDWP and JDI to link to JVM TI spec if you think appropriate. In the JVMTI spec for GetLoadedClasses: This suffers in a way similar to ClassLoaderReference.VisibleClasses in the JDWP spec, although not as badly. A simple concept ends up with a complex description, and it seems that description should really be in a more centralized place. I would also suggest a bit of cleanup of these lines: 6866 An array class is created directly by Java virtual machine. An array class 6867 creation can be triggered by using class loaders or by invoking methods in certain 6868 Java SE Platform API such as reflection. "Created by [the] Java virtual machine" (add "the") Change "An array class creation" to "The creation" since your are repeating "An array class" from the previous sentence. In the JVMTI spec ClassLoaderClasses section: "That is, initiating_loader has been recorded as an initiating loader of the returned classes." -> "That is, all classes for which initiating_loader has been recorded as an initiating loader." In the JVMTI spec GetClassSignature section: "If the class indicated by klass is ..." -> "If the class ..." (you just finished the previous sentence with "class indicated by Klass"). "the returned name is [the] JNI type signature" (add "the"). Also, is "JNI type signature" formally defined somewhere? This relates to my JDWP spec comment above. It's a link to https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. This is how the current JVM TI spec defines. " where N is the binary name encoded in internal form indicated by the class file". Is "binary name encoded in internal form" explained somewhere? JVMS 4.2.1 https://docs.oracle.com/javase/specs/jvms/se14/html/jvms-4.html#jvms-4.2.1 Also, can you add an example of a returned hidden class signature? OK In the JVMTI spec ClassLoad section: "representation using [a] class loader" (add "a") "By invoking Lookup::defineHiddenClass, that creates ..." -> "By invoking Lookup::defineHiddenClass to create ..." "certain Java SE Platform API" -> Should be "APIs" In JDI ClassLoaderReference.definedClasses() "loaded at least to the point of preparation and types ..." -> "loaded at least to the point of preparation, and types ..." (Note, this not a new issue with your edits) In Instrumentation.getAllLoadedClasses: The reference to `class` did not format properly. Serguei caught that one too. I fixed it in my local repo. "by invoking Lookup::defineHiddenClass that creates" -> "by invoking Lookup::defineHiddenClass, which creates" "An array class is created directly by Java virtual machine. An array class creation can be triggered ..." ->"An array class is created directly by the Java virtual machine. Array class creation can be triggered ..." In Instrumentation.getInitiatedClasses: "That is, loader has been recorded as an initiating loader of these classes." -> "That is, all classes for which loader has been recorded as an initiating loader." thanks,
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
On 4/16/20 9:45 AM, Mandy Chung wrote:
On 4/14/20 11:51 AM, Paul Sandoz wrote:
Looks good to me (not familiar with all the code areas.
Minor suggestion:
MethodHandles.java
1811 * ASCII periods. For the instance of {@link
java.lang.Class} representing {@code C}:
1812 *
1813 * {@link Class#getName()} returns the string
{@code GN + "/" + },
1814 * even though this is not a valid binary class or
interface name.
1815 * {@link Class#descriptorString()} returns the string
1816 * {@code "L" + N + ";" + "/" + },
1817 * even though this is not a valid type descriptor
name.
1818 *
Add another bullet:
“
even though this is not a valid type descriptor name; and
- therefore {@link Class#describeConstable} returns an empty {@code
Optional}.
“
?
OK. I add this bullet:
- Class.describeConstable() returns an empty optional as C cannot be
described in nominal form.
The webrev and spec was updated [1] for descriptor string to be of the
form "Lfoo/Foo.1234;" to mitigate the compatibility risk. Th
Specdiff with serviceability spec change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-inc/
Specdiff without svc spec change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-descriptor-string/overview-summary.html
Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-descriptor-string/
Svc spec change webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/
thanks
Mandy
[1]
https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-April/007155.html
Hi Mandy,
Thanks for updating the svc specs. Some comments below:
In the JDWP spec update, you changed "JNI signature" to "type signature"
in one place, but left it as "JNI signature" everywhere else. Should
they all be changed?
In the JDWP spec for ClassLoaderReference.VisibleClasses:
"That is, this class loader has been recorded as an initiating
loader of the returned classes." -> "That is, all classes for which this
class loader has been recorded as an initiating loader."
This seems like too much detail to be put here. Basically the term
"initiating ClassLoader" has turned into a short essay. Is it possible
that all this detail could be put elsewhere and referenced? Aren't there
other places in other specs where a similar clarification of "initiating
ClassLoader" is needed (I see now that ClassLoaderClasses in the JVMTI
spec, ClassLoaderReference,visibleClasses in the JDI spec, and
Instrumentation.getInitiatedClasses are all dealing with this, but not
all in the exact same way).
In the JVMTI spec for GetLoadedClasses:
This suffers in a way similar to ClassLoaderReference.VisibleClasses in
the JDWP spec, although not as badly. A simple concept ends up with a
complex description, and it seems that description should really be in a
more centralized place. I would also suggest a bit of cleanup of these
lines:
6866 An array class is created directly by Java virtual
machine. An array class
6867 creation can be triggered by using class loaders or by
invoking methods in certain
6868 Java SE Platform API such as reflection.
"Created by [the] Java virtual machine" (add "the")
Change "An array class creation" to "The creation" since your are
repeating "An array class" from the previous sentence.
In the JVMTI spec ClassLoaderClasses section:
"That is, initiating_loader has been recorded as an initiating loader of
the returned classes." -> "That is, all classes for which
initiating_loader has been recorded as an initiating loader."
In the JVMTI spec GetClassSignature section:
"If the class indicated by klass is ..." -> "If the class ..." (you just
finished the previous sentence with "class indicated by Klass").
"the returned name is [the] JNI type signature" (add "the"). Also, is
"JNI type signature" formally defined somewhere? This relates to my JDWP
spec comment above.
" where N is the binary name encoded in internal form indicated by the
class file". Is "binary name encoded in internal form" explained
somewhere? Also, can you add an example of a returned hidden class
signature?
In the JVMTI spec ClassLoad section:
"representation using [a] class loader" (add "a")
"By invoking Lookup::defineHiddenClass, that creates ..." -> "By
invoking Lookup::defineHiddenClass to create ..."
"certain Java SE Platform API" -> Should be "APIs"
In JDI ClassLoaderReference.definedClasses()
"loaded at least to the point of preparation and types ..." -> "loaded
at least to the point of preparation, and types ..." (Note, this not a
new issue with your edits)
In Instrumentation.getAllLoadedClasses:
The reference to `class` did not format properly.
"by invoking Lookup::defineHiddenClass that creates" -> "by invoking
Lookup::defineHiddenClass, which
Re: RFR: 8231585: java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java fails with java.lang.NullPointerException
Looks good. Chris On 4/17/20 1:03 PM, Daniil Titov wrote: Please review the change that fixes intermittent failure of java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java As David noticed (thank you, David, for this analysis) there is no guarantee that all threads found by getAllThreadIds() are still alive by the time we call getThreadInfo() so we have to allow for null array entries. Testing: Mach5 tests with Graal on passed 300 times. [1] http://cr.openjdk.java.net/~dtitov/8231585/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8231585 Best regards, Daniil
RFR: 8231585: java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java fails with java.lang.NullPointerException
Please review the change that fixes intermittent failure of java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java As David noticed (thank you, David, for this analysis) there is no guarantee that all threads found by getAllThreadIds() are still alive by the time we call getThreadInfo() so we have to allow for null array entries. Testing: Mach5 tests with Graal on passed 300 times. [1] http://cr.openjdk.java.net/~dtitov/8231585/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8231585 Best regards, Daniil
RFR(XS) 8242789: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with 'JShellToolProvider' missing from stdout/stderr
Hello, Please review the following: https://bugs.openjdk.java.net/browse/JDK-8242789 http://cr.openjdk.java.net/~cjplummer/8242789/webrev.00 JShellHeapDumpTest.java has two variants, one that does a short 2 second sleep after launching the jshell process (the main JShellHeapDumpTest.java test does this) and the other that does no sleep (HeapDumpTestWithActiveProcess.java does this by invoking JShellHeapDumpTest.java with the "nosleep" argument). The reason for the 2 second sleep is to get the jshell process into a steady state so JDK-8231634 [1] doesn't turn up when using SA on the jshell process. I added the sleep instead of problem listing JShellHeapDumpTest.java since it is a useful test even with the sleep in place. HeapDumpTestWithActiveProcess.java was added so we still had a test to reproduce JDK-8231634 [1], and was problem listed immediately. However, another side affect of not sleeping is sometimes SA requests the thread dump of the jshell process before jshell enters its main thread. Thus the test can't find the "JShellToolProvider" symbol in the thread dump. The fix is to simply not require the symbol to be present when in "nosleep" mode. thanks, Chris [1] https://bugs.openjdk.java.net/browse/JDK-8231634
Re: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)
Thanks Severin and David for your review.
Will push it tomorrow.
Best regards,
Jie
On 2020/4/17, 8:56 PM, "David Holmes" wrote:
On 17/04/2020 5:00 pm, jiefu(傅杰) wrote:
> Hi David,
>
> Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.02/
>
> The file header had been fixed. Please review it.
File header update looks good.
Thanks,
David
> Thanks a lot.
> Best regards,
> Jie
>
> On 2020/4/17, 11:59 AM, "David Holmes" wrote:
>
> Hi Jie,
>
> On 16/04/2020 11:23 pm, jiefu(傅杰) wrote:
> > Hi Severin,
> >
> > Thanks for your review and very nice suggestions.
> >
> > Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.01/
> >
> > test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java
is added to reproduce the bug.
>
>
> Can you please use the standard OpenJDK file header after your
Tencent
> copyright line:
>
>* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>*
>* This code is free software; you can redistribute it and/or
modify it
>* under the terms of the GNU General Public License version 2
only, as
>* published by the Free Software Foundation.
>*
>* This code is distributed in the hope that it will be useful, but
WITHOUT
>* ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or
>* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
License
>* version 2 for more details (a copy is included in the LICENSE
file that
>* accompanied this code).
>*
>* You should have received a copy of the GNU General Public License
> version
>* 2 along with this work; if not, write to the Free Software
Foundation,
>* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>*
>* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA
94065 USA
>* or visit www.oracle.com if you need additional information or
have any
>* questions.
>*/
>
> I don't think the "classpath exception" is relevant to tests -
certainly
> other tests I checked do not have it.
>
> Thanks,
> David
> -
>
> > Thanks a lot.
> > Best regards,
> > Jie
> >
> >
> > On 2020/4/16, 4:40 PM, "Severin Gehwolf"
wrote:
> >
> > Hi Jie,
> >
> > On Fri, 2020-04-10 at 01:49 +, jiefu(傅杰) wrote:
> > > Hi all,
> > >
> > > JBS:https://bugs.openjdk.java.net/browse/JDK-8242480
> > > Webrev: http://cr.openjdk.java.net/~jiefu/8242480/webrev.00/
> > >
> > > Negative values were returned by getFreeSwapSpaceSize() in
our docker testing.
> > > The reason is that current implementation doesn't consider
the situation when memory.limit_in_bytes == memory.memsw.limit_in_bytes, which
means do not use the swap space at all.
> > >
> > > In
src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java,
let's see
> > >
> > > 71 public long getFreeSwapSpaceSize() {
> > > 72 if (containerMetrics != null) {
> > > 73 long memSwapLimit =
containerMetrics.getMemoryAndSwapLimit();
> > > 74 long memLimit =
containerMetrics.getMemoryLimit();
> > > 75 if (memSwapLimit >= 0 && memLimit >= 0) {
> > > 76 for (int attempt = 0; attempt <
MAX_ATTEMPTS_NUMBER; attempt++) {
> > > 77 long memSwapUsage =
containerMetrics.getMemoryAndSwapUsage();
> > > 78 long memUsage =
containerMetrics.getMemoryUsage();
> > > 79 if (memSwapUsage > 0 && memUsage >
0) {
> > > 80 // We read "memory usage" and
"memory and swap usage" not atomically,
> > > 81 // and it's possible to get the
negative value when subtracting these two.
> > > 82 // If this happens just retry
the loop for a few iterations.
> > > 83 if ((memSwapUsage - memUsage)
>= 0) {
> > > 84 return memSwapLimit -
memLimit - (memSwapUsage - memUsage);
> > > 85 }
> > > 86 }
> > >
Re: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)
On 17/04/2020 5:00 pm, jiefu(傅杰) wrote:
Hi David,
Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.02/
The file header had been fixed. Please review it.
File header update looks good.
Thanks,
David
Thanks a lot.
Best regards,
Jie
On 2020/4/17, 11:59 AM, "David Holmes" wrote:
Hi Jie,
On 16/04/2020 11:23 pm, jiefu(傅杰) wrote:
> Hi Severin,
>
> Thanks for your review and very nice suggestions.
>
> Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.01/
>
> test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java is
added to reproduce the bug.
Can you please use the standard OpenJDK file header after your Tencent
copyright line:
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but
WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License
version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
I don't think the "classpath exception" is relevant to tests - certainly
other tests I checked do not have it.
Thanks,
David
-
> Thanks a lot.
> Best regards,
> Jie
>
>
> On 2020/4/16, 4:40 PM, "Severin Gehwolf" wrote:
>
> Hi Jie,
>
> On Fri, 2020-04-10 at 01:49 +, jiefu(傅杰) wrote:
> > Hi all,
> >
> > JBS:https://bugs.openjdk.java.net/browse/JDK-8242480
> > Webrev: http://cr.openjdk.java.net/~jiefu/8242480/webrev.00/
> >
> > Negative values were returned by getFreeSwapSpaceSize() in our
docker testing.
> > The reason is that current implementation doesn't consider the
situation when memory.limit_in_bytes == memory.memsw.limit_in_bytes, which means do
not use the swap space at all.
> >
> > In
src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java,
let's see
> >
> > 71 public long getFreeSwapSpaceSize() {
> > 72 if (containerMetrics != null) {
> > 73 long memSwapLimit =
containerMetrics.getMemoryAndSwapLimit();
> > 74 long memLimit = containerMetrics.getMemoryLimit();
> > 75 if (memSwapLimit >= 0 && memLimit >= 0) {
> > 76 for (int attempt = 0; attempt <
MAX_ATTEMPTS_NUMBER; attempt++) {
> > 77 long memSwapUsage =
containerMetrics.getMemoryAndSwapUsage();
> > 78 long memUsage =
containerMetrics.getMemoryUsage();
> > 79 if (memSwapUsage > 0 && memUsage > 0) {
> > 80 // We read "memory usage" and "memory and
swap usage" not atomically,
> > 81 // and it's possible to get the
negative value when subtracting these two.
> > 82 // If this happens just retry the
loop for a few iterations.
> > 83 if ((memSwapUsage - memUsage) >= 0) {
> > 84 return memSwapLimit - memLimit -
(memSwapUsage - memUsage);
> > 85 }
> > 86 }
> > 87 }
> > 88 }
> > 89 }
> > 90 return getFreeSwapSpaceSize0();
> > 91 }
> >
> > If memSwapLimit (@line 73) equals memLimit (@line 74), then
getFreeSwapSpaceSize() may return a negative value @line 84.
> >
> > It would be better to fix it.
> > Could you please review it and give me some advice?
>
> Would this be reproducible via a test? There is
> test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java which
> contains testOperatingSystemMXBeanAwareness() tests.
>
>
Re: RFR (T) 8242896: typo #ifdef INCLUDE_JVMTI in codeCache.cpp
On 4/17/20 4:58 AM, serguei.spit...@oracle.com wrote: Hi Coleen, LGTM++ On 4/16/20 18:47, David Holmes wrote: Hi Coleen, Still LGTM. The other guarded methods are only called from JVMTI code. The two that are now stubbed out would have been no-ops without JVMTI as old_compiled_method_table would have been NULL. Still seems trivial to me. This is not that trivial for me. :) But thank you for the comment, David. Thanks Serguei! Coleen Thanks, Serguei Thanks, David On 17/04/2020 1:14 am, coleen.phillim...@oracle.com wrote: On 4/16/20 10:24 AM, Magnus Ihse Bursie wrote: On 2020-04-16 04:37, coleen.phillim...@oracle.com wrote: On 4/15/20 9:37 PM, David Holmes wrote: Hi Coleen, On 16/04/2020 10:59 am, coleen.phillim...@oracle.com wrote: open webrev at http://cr.openjdk.java.net/~coleenp/2020/8242896.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8242896 Looks good but ... Built and ran vmTestbase RedefineTests which use the protected code. ... you need to ensure that builds that don't INCLUDE_JVMTI still work okay as they will now actually be excluding this code for the first time. Last time I tried to build minimal, it failed for some odd problem on my system. I'll wait until someone from the openjdk can build it then. thanks, Building minimal should work. Try something like this: "jib configure -- --with-jvm-variants=minimal". It should work. I used to get some error about some X11 library. I don't remember what it was. Now I get this warning: The following warnings were produced. Repeated here for convenience: WARNING: Ignoring value of PERL from the environment. Use command line variables instead. I'm not sure what it means but it seems to be building. I just tried with your patch, and it does not work. /localhome/hg/jdk-BAR/open/src/hotspot/share/classfile/metadataOnStackMark.cpp:70: error: undefined reference to 'CodeCache::old_nmethods_do(MetadataClosure*)' /localhome/hg/jdk-BAR/open/src/hotspot/share/code/nmethod.cpp:1495: error: undefined reference to 'CodeCache::unregister_old_nmethod(CompiledMethod*)' I made a quick check but it was not clear to me if the call sites in metadataOnStackMark.cpp and nmethod.cpp should be excluded if missing jvmti, or if the code in codeCache.cpp should really be present even with jvmti. They should be excluded. Thanks for testing it for me. Now it builds (up to that point). Hopefully still trivial. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8242896.02/webrev Thanks! Coleen /Magnus Coleen Thanks, David thanks, Coleen
Re: RFR (T) 8242896: typo #ifdef INCLUDE_JVMTI in codeCache.cpp
On 4/16/20 9:47 PM, David Holmes wrote: Hi Coleen, Still LGTM. The other guarded methods are only called from JVMTI code. The two that are now stubbed out would have been no-ops without JVMTI as old_compiled_method_table would have been NULL. Yes, that is true. I considered #if INCLUDE_JVMTI around them but then I'd have to change another file. Still seems trivial to me. Thanks, Turned out a bit less trivial than the original 3 characters. Coleen Thanks, David On 17/04/2020 1:14 am, coleen.phillim...@oracle.com wrote: On 4/16/20 10:24 AM, Magnus Ihse Bursie wrote: On 2020-04-16 04:37, coleen.phillim...@oracle.com wrote: On 4/15/20 9:37 PM, David Holmes wrote: Hi Coleen, On 16/04/2020 10:59 am, coleen.phillim...@oracle.com wrote: open webrev at http://cr.openjdk.java.net/~coleenp/2020/8242896.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8242896 Looks good but ... Built and ran vmTestbase RedefineTests which use the protected code. ... you need to ensure that builds that don't INCLUDE_JVMTI still work okay as they will now actually be excluding this code for the first time. Last time I tried to build minimal, it failed for some odd problem on my system. I'll wait until someone from the openjdk can build it then. thanks, Building minimal should work. Try something like this: "jib configure -- --with-jvm-variants=minimal". It should work. I used to get some error about some X11 library. I don't remember what it was. Now I get this warning: The following warnings were produced. Repeated here for convenience: WARNING: Ignoring value of PERL from the environment. Use command line variables instead. I'm not sure what it means but it seems to be building. I just tried with your patch, and it does not work. /localhome/hg/jdk-BAR/open/src/hotspot/share/classfile/metadataOnStackMark.cpp:70: error: undefined reference to 'CodeCache::old_nmethods_do(MetadataClosure*)' /localhome/hg/jdk-BAR/open/src/hotspot/share/code/nmethod.cpp:1495: error: undefined reference to 'CodeCache::unregister_old_nmethod(CompiledMethod*)' I made a quick check but it was not clear to me if the call sites in metadataOnStackMark.cpp and nmethod.cpp should be excluded if missing jvmti, or if the code in codeCache.cpp should really be present even with jvmti. They should be excluded. Thanks for testing it for me. Now it builds (up to that point). Hopefully still trivial. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8242896.02/webrev Thanks! Coleen /Magnus Coleen Thanks, David thanks, Coleen
Re: RFR (T) 8242896: typo #ifdef INCLUDE_JVMTI in codeCache.cpp
Hi Coleen, LGTM++ On 4/16/20 18:47, David Holmes wrote: Hi Coleen, Still LGTM. The other guarded methods are only called from JVMTI code. The two that are now stubbed out would have been no-ops without JVMTI as old_compiled_method_table would have been NULL. Still seems trivial to me. This is not that trivial for me. :) But thank you for the comment, David. Thanks, Serguei Thanks, David On 17/04/2020 1:14 am, coleen.phillim...@oracle.com wrote: On 4/16/20 10:24 AM, Magnus Ihse Bursie wrote: On 2020-04-16 04:37, coleen.phillim...@oracle.com wrote: On 4/15/20 9:37 PM, David Holmes wrote: Hi Coleen, On 16/04/2020 10:59 am, coleen.phillim...@oracle.com wrote: open webrev at http://cr.openjdk.java.net/~coleenp/2020/8242896.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8242896 Looks good but ... Built and ran vmTestbase RedefineTests which use the protected code. ... you need to ensure that builds that don't INCLUDE_JVMTI still work okay as they will now actually be excluding this code for the first time. Last time I tried to build minimal, it failed for some odd problem on my system. I'll wait until someone from the openjdk can build it then. thanks, Building minimal should work. Try something like this: "jib configure -- --with-jvm-variants=minimal". It should work. I used to get some error about some X11 library. I don't remember what it was. Now I get this warning: The following warnings were produced. Repeated here for convenience: WARNING: Ignoring value of PERL from the environment. Use command line variables instead. I'm not sure what it means but it seems to be building. I just tried with your patch, and it does not work. /localhome/hg/jdk-BAR/open/src/hotspot/share/classfile/metadataOnStackMark.cpp:70: error: undefined reference to 'CodeCache::old_nmethods_do(MetadataClosure*)' /localhome/hg/jdk-BAR/open/src/hotspot/share/code/nmethod.cpp:1495: error: undefined reference to 'CodeCache::unregister_old_nmethod(CompiledMethod*)' I made a quick check but it was not clear to me if the call sites in metadataOnStackMark.cpp and nmethod.cpp should be excluded if missing jvmti, or if the code in codeCache.cpp should really be present even with jvmti. They should be excluded. Thanks for testing it for me. Now it builds (up to that point). Hopefully still trivial. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8242896.02/webrev Thanks! Coleen /Magnus Coleen Thanks, David thanks, Coleen
Re: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)
On Fri, 2020-04-17 at 06:58 +, jiefu(傅杰) wrote: > Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.02/ Looks good. Thanks, Severin
Re: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)
Hi David,
Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.02/
The file header had been fixed. Please review it.
Thanks a lot.
Best regards,
Jie
On 2020/4/17, 11:59 AM, "David Holmes" wrote:
Hi Jie,
On 16/04/2020 11:23 pm, jiefu(傅杰) wrote:
> Hi Severin,
>
> Thanks for your review and very nice suggestions.
>
> Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.01/
>
> test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java is
added to reproduce the bug.
Can you please use the standard OpenJDK file header after your Tencent
copyright line:
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License
version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
I don't think the "classpath exception" is relevant to tests - certainly
other tests I checked do not have it.
Thanks,
David
-
> Thanks a lot.
> Best regards,
> Jie
>
>
> On 2020/4/16, 4:40 PM, "Severin Gehwolf" wrote:
>
> Hi Jie,
>
> On Fri, 2020-04-10 at 01:49 +, jiefu(傅杰) wrote:
> > Hi all,
> >
> > JBS:https://bugs.openjdk.java.net/browse/JDK-8242480
> > Webrev: http://cr.openjdk.java.net/~jiefu/8242480/webrev.00/
> >
> > Negative values were returned by getFreeSwapSpaceSize() in our
docker testing.
> > The reason is that current implementation doesn't consider the
situation when memory.limit_in_bytes == memory.memsw.limit_in_bytes, which
means do not use the swap space at all.
> >
> > In
src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java,
let's see
> >
> > 71 public long getFreeSwapSpaceSize() {
> > 72 if (containerMetrics != null) {
> > 73 long memSwapLimit =
containerMetrics.getMemoryAndSwapLimit();
> > 74 long memLimit = containerMetrics.getMemoryLimit();
> > 75 if (memSwapLimit >= 0 && memLimit >= 0) {
> > 76 for (int attempt = 0; attempt <
MAX_ATTEMPTS_NUMBER; attempt++) {
> > 77 long memSwapUsage =
containerMetrics.getMemoryAndSwapUsage();
> > 78 long memUsage =
containerMetrics.getMemoryUsage();
> > 79 if (memSwapUsage > 0 && memUsage > 0) {
> > 80 // We read "memory usage" and "memory
and swap usage" not atomically,
> > 81 // and it's possible to get the
negative value when subtracting these two.
> > 82 // If this happens just retry the loop
for a few iterations.
> > 83 if ((memSwapUsage - memUsage) >= 0) {
> > 84 return memSwapLimit - memLimit -
(memSwapUsage - memUsage);
> > 85 }
> > 86 }
> > 87 }
> > 88 }
> > 89 }
> > 90 return getFreeSwapSpaceSize0();
> > 91 }
> >
> > If memSwapLimit (@line 73) equals memLimit (@line 74), then
getFreeSwapSpaceSize() may return a negative value @line 84.
> >
> > It would be better to fix it.
> > Could you please review it and give me some advice?
>
> Would this be reproducible via a test? There is
> test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java which
> contains testOperatingSystemMXBeanAwareness() tests.
>
> It would be good to capture this in a test somehow.
>
> Thanks,
> Severin
>
>
>
>
Re: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)
Hi Severin, Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.02/ Please review it. Thanks a lot. Best regards, Jie On 2020/4/16, 11:40 PM, "Severin Gehwolf" wrote: Since you've added a new test, please move them to the jdk docker tests in: test/jdk/jdk/internal/platform/docker/ Fixed. test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java + * @build sun.hotspot.WhiteBox GetFreeSwapSpaceSize + * @run driver ClassFileInstaller -jar whitebox.jar sun.hotspot.WhiteBox sun.hotspot.WhiteBox$WhiteBoxPermission I don't see any reason why WhiteBox would be needed for this test. Is that an oversight or am I missing something? It's my oversight. Thanks for correcting me.
