Re: PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Hi Yasumasa,
On 20/04/2016 7:15 PM, Yasumasa Suenaga wrote:
Hi David,
... on 32-bit size_t and julong are not the same size so we would
still need to ensure we don't specify a filesize that is greater than
SIZE_MAX on 32-bit.
Oh... I understood.
I've fixed and uploaded new webrev. Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.03/
So it just registered with me that currently filesize is interpreted as
a value in KB. With this change it will be in bytes - that means tests
will need fixing eg:
hotspot/test/serviceability/logging/TestLogRotation.java
That change in semantics may not be desirable, but I'll leave that to
the owners of this code to decide (and I hope they jump in soon!)
I note that in the existing range check:
if (value == SIZE_MAX || value > SIZE_MAX / K) {
the first clause is redundant. So your change seems okay.
Thanks,
David
-
Thanks,
Yasumasa
On 2016/04/20 15:04, David Holmes wrote:
On 20/04/2016 3:25 PM, Yasumasa Suenaga wrote:
Hi David,
> You still removed the size bounds checks:
>
> 190 size_t value = parse_value(value_str);
> 191 if (value == SIZE_MAX || value > SIZE_MAX / K) {
> 192 errstream->print_cr("Invalid option: %s must be in range
[0, "
> 193 SIZE_FORMAT "]", FileSizeOptionKey,
SIZE_MAX / K);
> 194 success = false;
SIZE_MAX is defined as ULONG_MAX in stdint.h [1].
Ah I hadn't realized this was an external value, I thought it was some
internally enforced maximum file size limit. So this is just an
overflow check really, and ...
Arguments::atojulong(atomull) checks value range [2].
... we already do an overflow check in here, but ...
Thus I do not think we do not need to check value range in
LogFileOutput::parse_options().
... on 32-bit size_t and julong are not the same size so we would
still need to ensure we don't specify a filesize that is greater than
SIZE_MAX on 32-bit.
> Thanks, I had missed that example usage buried in there, but am still
> surprised none of these "options" for the handling the file are
> explicitly documented.
I do not know how we can documented about it.
(Is it internal process in Oracle?)
No I just meant that amongst all that help text you already modified,
there is nothing, that I could see, that actually describes the
possible options for filesize.
Thanks,
David
I can help for it if I can
Thanks,
Yasumasa
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/generic/stdint.h;h=442762728b899aa8ec219299692fce5953d796b0;hb=HEAD#l259
[2]
http://hg.openjdk.java.net/jdk9/hs/hotspot/file/8005261869c9/src/share/vm/runtime/arguments.cpp#l804
2016/04/20 11:24 "David Holmes" >:
Hi Yasumasa,
On 19/04/2016 11:50 PM, Yasumasa Suenaga wrote:
> Hi David,
>
> Thank you for your comment.
>
> I uploaded new webrev. Could you review again?
> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.02/
You still removed the size bounds checks:
190 size_t value = parse_value(value_str);
191 if (value == SIZE_MAX || value > SIZE_MAX / K) {
192 errstream->print_cr("Invalid option: %s must be in
range [0, "
193 SIZE_FORMAT "]",
FileSizeOptionKey,
SIZE_MAX / K);
194 success = false;
>> Which makes me wonder if atomull needs renaming - does the
"m" mean
>> memory? atojulong would seem more appropriate regardless.
>
> I renamed to atojulong() in new webrev.
>
>> Not directly related to your change but I was surprised that the
>> various log file options don't seem to be documented anywhere
in the
>> -Xlog:help output.
>
> I updated help message in new webrev.
Thanks, I had missed that example usage buried in there, but am
still
surprised none of these "options" for the handling the file are
explicitly documented.
Thanks,
David
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/04/19 10:14, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 19/04/2016 12:06 AM, Yasumasa Suenaga wrote:
>>> PING:
>>>
>>> I've sent review request for JDK-8153073.
>>> Could you review it?
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>
>>> If this patch is merged, user can set logfile size with k/m/g.
>>
>> Your webrev seems out of date with respect to the current
code - the
>> logfile size processing is done in
LogFileOutput::parse_options not
>> configure_rotation. And of course you now need to work with
jdk9/hs not
>> hs-rt.
>>
>> That aside:
>>
>> src/share/vm/runtime/arguments.cpp
>>
>> I don't think you need to add the Arguments:: to the atomull
calls when
>> you are executing in Arguments code
RFR (XS): 8154794,,Add support for experimental fields/events to event-based tracing
Hi, Could I have a review of this small fix that adds support for experimental events and experimental fields for event based tracing. Bug: https://bugs.openjdk.java.net/browse/JDK-8154794 Webrev: http://cr.openjdk.java.net/~egahlin/8154794/ Thanks Erik
Re: RFR(S): JDK-8143921 nsk/jdi/ObjectReference/waitingThreads/waitingthreads003 fails with JVMTI_ERROR_INVALID_CLASS
Dmitry,
Thank you for the reply.
I hope, you run all the JDI tests including nsk.jdi and jtreg com/sun/jdi.
Reviewed.
Thanks,
Serguei
On 4/20/16 10:12, Dmitry Samersoff wrote:
Serguei,
The test fails rarely and I'm not able to reproduce the issue.
JDWP call JvmtiGetLoadedClasses::getLoadedClasses then store received
classes locally.
So nothing prevent these classes from being unloaded and I think it's
worth to fix.
-Dmitry
On 2016-04-19 19:49, serguei.spit...@oracle.com wrote:
On 4/19/16 09:42, serguei.spit...@oracle.com wrote:
Hi Dmitry,
144 // Clazz become invalid since the time we get the class list
145 // Skip this entry
146 if (error == JVMTI_ERROR_INVALID_CLASS) {
147 continue;
148 }
149
I'd like to better understand the root cause of this issue.
Could you, please, give an idea why a class in this test becomes invalid?
Do you think there is a potential in this test for a class to get
unloaded?
Or is it something about anonymous classes?
One more question:
Any ideas on why these two tests started failing in November 2015 and
did not fail before?
Thanks,
Serguei
Thanks,
Serguei
On 4/19/16 06:56, Dmitry Samersoff wrote:
Any reviewers?
On 2016-04-13 19:24, Dmitry Samersoff wrote:
Everybody.
Please review a small fix.
http://cr.openjdk.java.net/~dsamersoff/JDK-8143921/webrev.01/
I don't have a reproducer, so the fix is based on coredump analyzes.
Tested with rbt
-Dmitry
Re: RFR(S): JDK-8143921 nsk/jdi/ObjectReference/waitingThreads/waitingthreads003 fails with JVMTI_ERROR_INVALID_CLASS
Serguei,
The test fails rarely and I'm not able to reproduce the issue.
JDWP call JvmtiGetLoadedClasses::getLoadedClasses then store received
classes locally.
So nothing prevent these classes from being unloaded and I think it's
worth to fix.
-Dmitry
On 2016-04-19 19:49, serguei.spit...@oracle.com wrote:
> On 4/19/16 09:42, serguei.spit...@oracle.com wrote:
>> Hi Dmitry,
>>
>> 144 // Clazz become invalid since the time we get the class list
>> 145 // Skip this entry
>> 146 if (error == JVMTI_ERROR_INVALID_CLASS) {
>> 147 continue;
>> 148 }
>> 149
>> I'd like to better understand the root cause of this issue.
>> Could you, please, give an idea why a class in this test becomes invalid?
>> Do you think there is a potential in this test for a class to get
>> unloaded?
>> Or is it something about anonymous classes?
>
> One more question:
> Any ideas on why these two tests started failing in November 2015 and
> did not fail before?
>
>
> Thanks,
> Serguei
>
>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 4/19/16 06:56, Dmitry Samersoff wrote:
>>> Any reviewers?
>>>
>>> On 2016-04-13 19:24, Dmitry Samersoff wrote:
Everybody.
Please review a small fix.
http://cr.openjdk.java.net/~dsamersoff/JDK-8143921/webrev.01/
I don't have a reproducer, so the fix is based on coredump analyzes.
Tested with rbt
-Dmitry
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
RFR:8153989:Some SVC tests fail on compact2 due to an unnecessary test library dependency
Hi, Could you, please, review this small tests-only fix: CR: https://bugs.openjdk.java.net/browse/JDK-8153992 "Some SVC tests fail on compact2 due to an unnecessary test library dependency" Webrev: http://cr.openjdk.java.net/~akulyakh/8153992/test/testlibrary/jdk/test/lib/ProcessTools.java.udiff.html Before the fix the ProcessTools.getProcessId() used the ManagementFactory.getRuntimeMXBean() API. The API is not available on compact2 and below. Therefore the tests failed. We are changing the ProcessTools.getProcessId() method to use the JDK 9 Process.getPid(). This eliminates the unnecessary dependency making the tests pass on compact2. I am not sure how acceptable it is to cast from long to int this change. If it is not acceptable we can change the return type to long. This however, will cause massive changes throughout the hotspot tests which presently expect getProcessId() to return int. Best regards, Alexander
Re: PING: RFR: JDK-8151815: Could not parse core image with JSnap.
Yasumasa,
I need some more time to check what is happening with performance
counters and what side effect this fix can have.
Sorry about it.
-Dmitry
On 2016-04-20 15:14, Yasumasa Suenaga wrote:
> PING: Could you review it?
>
>>http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
>
> This changes is based on jdk9/hs-rt.
> But I confirmed this patch works fine on jdk9/hs .
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/04/13 22:22, Yasumasa Suenaga wrote:
>> Hi Dmitry,
>>
>> Thank you for your comment.
>> I uploaded new webrev. Could you review again?
>>
>>http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2016/04/13 21:41, Dmitry Samersoff wrote:
>>> Yasumasa,
>>>
>>> Yes. It's better.
>>>
>>> Please place all _saved_* declarations together and add a comment
>>> explaining what is the purpose of this variables.
>>>
>>> -Dmitry
>>>
>>>
>>> On 2016-04-13 15:20, Yasumasa Suenaga wrote:
Hi all,
Could you review and sponsor it?
> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.01/
If it is not accepted, I think that we can add new field to PerfMemory
for using in JSnap:
---
diff -r 4823056a5bbd src/share/vm/runtime/perfMemory.cpp
--- a/src/share/vm/runtime/perfMemory.cppTue Apr 12 09:08:48
2016 +
+++ b/src/share/vm/runtime/perfMemory.cppWed Apr 13 14:18:15
2016 +0900
@@ -45,11 +45,16 @@
UINT_CHARS + 1;
char*PerfMemory::_start = NULL;
+char*PerfMemory::_saved_start = NULL;
char*PerfMemory::_end = NULL;
+char*PerfMemory::_saved_end = NULL;
char*PerfMemory::_top = NULL;
+char*PerfMemory::_saved_top = NULL;
size_t PerfMemory::_capacity = 0;
+size_t PerfMemory::_saved_capacity = 0;
jint PerfMemory::_initialized = false;
PerfDataPrologue*PerfMemory::_prologue = NULL;
+PerfDataPrologue*PerfMemory::_saved_prologue = NULL;
void perfMemory_init() {
@@ -103,6 +108,8 @@
// allocate PerfData memory region
create_memory_region(capacity);
+ _saved_start = _start;
+ _saved_capacity = _capacity;
if (_start == NULL) {
@@ -132,8 +139,11 @@
}
_prologue = (PerfDataPrologue *)_start;
+_saved_prologue = _prologue;
_end = _start + _capacity;
+_saved_end = _end;
_top = _start + sizeof(PerfDataPrologue);
+_saved_top = _top;
}
assert(_prologue != NULL, "prologue pointer must be initialized");
---
If it is better, I will upload a webrev.
Thanks,
Yasumasa
On 2016/04/06 12:42, Yasumasa Suenaga wrote:
> Hi Dmitry,
>
> Thanks for your comment.
>
> I uploaded a new webrev. Could you review again?
> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.01/
>
> On 2016/04/06 0:31, Dmitry Samersoff wrote:
>> Yasumasa,
>>
>> 1. It's better to change JSnap code to produce meaningful error
>> message
>> instead of NPE.
>
> I added null check and set message to PerfMemory.java .
>
>
>> 2. We should check that no other consumer of perf counters rely on
>> the
>> fact it's NULL after call to destroy(). I'm not sure that this
>> part of
>> the fix is not dangerous.
>
> I added new flag (_destroyed) in PerfMemory class.
> This flag set true at PerfMemory::destroy().
>
> _start, _end, and more field which I removed from destroy() are
> private member of
> PerfMemory. I implemented that the getter functions of them check
> _destroyed flag,
> and returns same value before this change.
>
> So I think this change does not affect other place.
>
>
> Thanks,
>
> Yasumasa
>
>
>> -Dmitry
>>
>> On 2016-03-29 15:02, Yasumasa Suenaga wrote:
>>> PING: Could you review it?
>>>
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/03/22 21:24, Yasumasa Suenaga wrote:
PING: Could you review it?
Yasumasa
2016/03/14 23:39 "Yasumasa Suenaga" >:
Hi all,
When I use `jhsdb jsnap` to get PerfCounter from core
images, I
encountered NPE:
-
Exception in thread "main" java.lang.NullPointerException
at sun.jvm.hotspot.tools.JSnap.run(JSnap.java:45)
at
PING: RFR: JDK-8151815: Could not parse core image with JSnap.
PING: Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
This changes is based on jdk9/hs-rt.
But I confirmed this patch works fine on jdk9/hs .
Thanks,
Yasumasa
On 2016/04/13 22:22, Yasumasa Suenaga wrote:
Hi Dmitry,
Thank you for your comment.
I uploaded new webrev. Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
Thanks,
Yasumasa
On 2016/04/13 21:41, Dmitry Samersoff wrote:
Yasumasa,
Yes. It's better.
Please place all _saved_* declarations together and add a comment
explaining what is the purpose of this variables.
-Dmitry
On 2016-04-13 15:20, Yasumasa Suenaga wrote:
Hi all,
Could you review and sponsor it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.01/
If it is not accepted, I think that we can add new field to PerfMemory
for using in JSnap:
---
diff -r 4823056a5bbd src/share/vm/runtime/perfMemory.cpp
--- a/src/share/vm/runtime/perfMemory.cppTue Apr 12 09:08:48 2016 +
+++ b/src/share/vm/runtime/perfMemory.cppWed Apr 13 14:18:15 2016 +0900
@@ -45,11 +45,16 @@
UINT_CHARS + 1;
char*PerfMemory::_start = NULL;
+char*PerfMemory::_saved_start = NULL;
char*PerfMemory::_end = NULL;
+char*PerfMemory::_saved_end = NULL;
char*PerfMemory::_top = NULL;
+char*PerfMemory::_saved_top = NULL;
size_t PerfMemory::_capacity = 0;
+size_t PerfMemory::_saved_capacity = 0;
jint PerfMemory::_initialized = false;
PerfDataPrologue*PerfMemory::_prologue = NULL;
+PerfDataPrologue*PerfMemory::_saved_prologue = NULL;
void perfMemory_init() {
@@ -103,6 +108,8 @@
// allocate PerfData memory region
create_memory_region(capacity);
+ _saved_start = _start;
+ _saved_capacity = _capacity;
if (_start == NULL) {
@@ -132,8 +139,11 @@
}
_prologue = (PerfDataPrologue *)_start;
+_saved_prologue = _prologue;
_end = _start + _capacity;
+_saved_end = _end;
_top = _start + sizeof(PerfDataPrologue);
+_saved_top = _top;
}
assert(_prologue != NULL, "prologue pointer must be initialized");
---
If it is better, I will upload a webrev.
Thanks,
Yasumasa
On 2016/04/06 12:42, Yasumasa Suenaga wrote:
Hi Dmitry,
Thanks for your comment.
I uploaded a new webrev. Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.01/
On 2016/04/06 0:31, Dmitry Samersoff wrote:
Yasumasa,
1. It's better to change JSnap code to produce meaningful error message
instead of NPE.
I added null check and set message to PerfMemory.java .
2. We should check that no other consumer of perf counters rely on the
fact it's NULL after call to destroy(). I'm not sure that this part of
the fix is not dangerous.
I added new flag (_destroyed) in PerfMemory class.
This flag set true at PerfMemory::destroy().
_start, _end, and more field which I removed from destroy() are
private member of
PerfMemory. I implemented that the getter functions of them check
_destroyed flag,
and returns same value before this change.
So I think this change does not affect other place.
Thanks,
Yasumasa
-Dmitry
On 2016-03-29 15:02, Yasumasa Suenaga wrote:
PING: Could you review it?
Yasumasa
On 2016/03/22 21:24, Yasumasa Suenaga wrote:
PING: Could you review it?
Yasumasa
2016/03/14 23:39 "Yasumasa Suenaga" >:
Hi all,
When I use `jhsdb jsnap` to get PerfCounter from core images, I
encountered NPE:
-
Exception in thread "main" java.lang.NullPointerException
at sun.jvm.hotspot.tools.JSnap.run(JSnap.java:45)
at
sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:260)
at sun.jvm.hotspot.tools.Tool.start(Tool.java:223)
at sun.jvm.hotspot.tools.Tool.execute(Tool.java:118)
at sun.jvm.hotspot.tools.JSnap.main(JSnap.java:67)
at
sun.jvm.hotspot.SALauncher.runJSNAP(SALauncher.java:352)
at sun.jvm.hotspot.SALauncher.main(SALauncher.java:404)
-
PerfMemory::destroy() clears all members which are used in JSnap.
Thus NPE is occurred.
I uploaded webrev for this issue.
Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.00/
I cannot access JPRT.
So I need a Sponsor.
Thanks,
Yasumasa
Re: RFR: 8154719: JvmtiBreakpoint rename method print() to print_on()
Thanks Marcus! /Robbin On 04/20/2016 01:54 PM, Marcus Larsson wrote: Hi, On 04/20/2016 01:47 PM, Robbin Ehn wrote: Hi all, please review! print() method was changed in 8154041 to have outputStream argument, this renames print to print_on. Since JvmtiBreakpoint extends GrowableElement print_on already exists as a virtual const method, hence the const changes. Bug: https://bugs.openjdk.java.net/browse/JDK-8154719 Webrev: http://cr.openjdk.java.net/~rehn/8154719/webrev/ Looks good to me. Thanks, Marcus Tested with: #define TraceJVMTICalls false in hotspot/src/share/vm/prims/jvmtiEnv.cpp set to true with test-set nsk.jvmti.SetBreakpoint Thanks! /Robbin
Re: RFR: 8154719: JvmtiBreakpoint rename method print() to print_on()
Hi, On 04/20/2016 01:47 PM, Robbin Ehn wrote: Hi all, please review! print() method was changed in 8154041 to have outputStream argument, this renames print to print_on. Since JvmtiBreakpoint extends GrowableElement print_on already exists as a virtual const method, hence the const changes. Bug: https://bugs.openjdk.java.net/browse/JDK-8154719 Webrev: http://cr.openjdk.java.net/~rehn/8154719/webrev/ Looks good to me. Thanks, Marcus Tested with: #define TraceJVMTICalls false in hotspot/src/share/vm/prims/jvmtiEnv.cpp set to true with test-set nsk.jvmti.SetBreakpoint Thanks! /Robbin
RFR: 8154719: JvmtiBreakpoint rename method print() to print_on()
Hi all, please review! print() method was changed in 8154041 to have outputStream argument, this renames print to print_on. Since JvmtiBreakpoint extends GrowableElement print_on already exists as a virtual const method, hence the const changes. Bug: https://bugs.openjdk.java.net/browse/JDK-8154719 Webrev: http://cr.openjdk.java.net/~rehn/8154719/webrev/ Tested with: #define TraceJVMTICalls false in hotspot/src/share/vm/prims/jvmtiEnv.cpp set to true with test-set nsk.jvmti.SetBreakpoint Thanks! /Robbin
Re: PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
Hi David,
... on 32-bit size_t and julong are not the same size so we would still need to
ensure we don't specify a filesize that is greater than SIZE_MAX on 32-bit.
Oh... I understood.
I've fixed and uploaded new webrev. Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.03/
Thanks,
Yasumasa
On 2016/04/20 15:04, David Holmes wrote:
On 20/04/2016 3:25 PM, Yasumasa Suenaga wrote:
Hi David,
> You still removed the size bounds checks:
>
> 190 size_t value = parse_value(value_str);
> 191 if (value == SIZE_MAX || value > SIZE_MAX / K) {
> 192 errstream->print_cr("Invalid option: %s must be in range
[0, "
> 193 SIZE_FORMAT "]", FileSizeOptionKey,
SIZE_MAX / K);
> 194 success = false;
SIZE_MAX is defined as ULONG_MAX in stdint.h [1].
Ah I hadn't realized this was an external value, I thought it was some
internally enforced maximum file size limit. So this is just an overflow check
really, and ...
Arguments::atojulong(atomull) checks value range [2].
... we already do an overflow check in here, but ...
Thus I do not think we do not need to check value range in
LogFileOutput::parse_options().
... on 32-bit size_t and julong are not the same size so we would still need to
ensure we don't specify a filesize that is greater than SIZE_MAX on 32-bit.
> Thanks, I had missed that example usage buried in there, but am still
> surprised none of these "options" for the handling the file are
> explicitly documented.
I do not know how we can documented about it.
(Is it internal process in Oracle?)
No I just meant that amongst all that help text you already modified, there is
nothing, that I could see, that actually describes the possible options for
filesize.
Thanks,
David
I can help for it if I can
Thanks,
Yasumasa
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/generic/stdint.h;h=442762728b899aa8ec219299692fce5953d796b0;hb=HEAD#l259
[2]
http://hg.openjdk.java.net/jdk9/hs/hotspot/file/8005261869c9/src/share/vm/runtime/arguments.cpp#l804
2016/04/20 11:24 "David Holmes" >:
Hi Yasumasa,
On 19/04/2016 11:50 PM, Yasumasa Suenaga wrote:
> Hi David,
>
> Thank you for your comment.
>
> I uploaded new webrev. Could you review again?
> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.02/
You still removed the size bounds checks:
190 size_t value = parse_value(value_str);
191 if (value == SIZE_MAX || value > SIZE_MAX / K) {
192 errstream->print_cr("Invalid option: %s must be in
range [0, "
193 SIZE_FORMAT "]", FileSizeOptionKey,
SIZE_MAX / K);
194 success = false;
>> Which makes me wonder if atomull needs renaming - does the "m" mean
>> memory? atojulong would seem more appropriate regardless.
>
> I renamed to atojulong() in new webrev.
>
>> Not directly related to your change but I was surprised that the
>> various log file options don't seem to be documented anywhere in the
>> -Xlog:help output.
>
> I updated help message in new webrev.
Thanks, I had missed that example usage buried in there, but am still
surprised none of these "options" for the handling the file are
explicitly documented.
Thanks,
David
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/04/19 10:14, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 19/04/2016 12:06 AM, Yasumasa Suenaga wrote:
>>> PING:
>>>
>>> I've sent review request for JDK-8153073.
>>> Could you review it?
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>
>>> If this patch is merged, user can set logfile size with k/m/g.
>>
>> Your webrev seems out of date with respect to the current code - the
>> logfile size processing is done in LogFileOutput::parse_options not
>> configure_rotation. And of course you now need to work with
jdk9/hs not
>> hs-rt.
>>
>> That aside:
>>
>> src/share/vm/runtime/arguments.cpp
>>
>> I don't think you need to add the Arguments:: to the atomull
calls when
>> you are executing in Arguments code - lines 2643, 2660
>>
>> This comment could be updated to delete "memory"
>>
>>788 // Parses a memory size specification string.
>>
>> Which makes me wonder if atomull needs renaming - does the "m" mean
>> memory? atojulong would seem more appropriate regardless.
>>
>> ---
>>
>> src/share/vm/logging/logFileOutput.cpp
>>
>> You removed the size checking logic.
>>
>> Not directly related to your change but I was surprised that the
>> various log file options don't seem to be documented anywhere in the
>>
Re: RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command
On Tue, 2016-04-19 at 19:32 -0700, serguei.spit...@oracle.com wrote: > On 4/19/16 19:06, serguei.spit...@oracle.com wrote: > > > > On 4/19/16 02:22, serguei.spit...@oracle.com wrote: > > > > > > On 4/19/16 02:16, Severin Gehwolf wrote: > > > > > > > > Hi, > > > > > > > > On Tue, 2016-04-19 at 01:33 -0700, serguei.spit...@oracle.com wrote: > > > > > > > > > > Hi Severin, > > > > > > > > > > Please, find my comments below. > > > > > > > > > > On 4/15/16 13:52, serguei.spit...@oracle.com wrote: > > > > > > > > > > > > On 4/15/16 11:59, Dmitry Samersoff wrote: > > > > > > > > > > > > > > Severin, > > > > > > > > > > > > > > Looks good for me. > > > > > > > > > > > > > > But I'm a little afraid of the fact that now we are holding > > > > > > > eventHandler_lock while doing invoke*. > > > > > > It seems, I have this concern too. > > > > > > Please, let me take a look closer at this part if it is done in a > > > > > > right way. > > > > > Ok, my question was not about the eventHandler_lock, but about > > > > > the invokerLock. Please, see below. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So please hold on with backports until the fix bakes in jdk9 for > > > > > > > some time. > > > > > > +1 > > > > > > Thanks, > > > > > > Serguei > > > > > > > > > > > > > > > > > > > > -Dmitry > > > > > > > > > > > > > > On 2016-04-15 19:53, Severin Gehwolf wrote: > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > Here is a patch which is a redo of the fix for JDK-4858370 > > > > > > > > which > > > > > > > > got > > > > > > > > backed out due to it causing test regressions. Specifically > > > > > > > > problems > > > > > > > > were reported for com/sun/jdi/InvokeTest.java > > > > > > > > and com/sun/jdi/InterfaceMethodsTest.java with the fix for > > > > > > > > JDK-4858370 > > > > > > > > applied. > > > > > > > > > > > > > > > > Those test regressions were caused because: > > > > > > > > a.) The fix for JDK-4858370 deleted refs from the request object > > > > > > > > while *not* holding the invoker lock. > > > > > > > > b.) Invokes done via invoker_doInvoke() save global references, > > > > > > > > but > > > > > > > > don't hold the invoker lock either. > > > > > It seems, the invokerLock is to protect any uses of the 'request' > > > > > pointer that points > > > > > to the field ThreadNode.currentInvoke, not to protect the > > > > > saveGlobalRef() call itself. > > > > > So that, we have a hole in synchronization that you nicely discovered. > > > > Yes, that's right. saveGlobalRef() saves references by using the > > > > request pointer. > > > > > > > > > > > > > > Here is a couple of more places where the 'request' pointer is not > > > > > protected with > > > > > the invokerLock: > > > > > invoker_enableInvokeRequests(), invoker_isPending(), > > > > > invoker_isEnabled(). > > > > > > > > > > I've filed a new bug: > > > > > https://bugs.openjdk.java.net/browse/JDK-8154529 > > > > > > > > > > BTW, these are pretty simple or rare cases that do not harm much. > > > > > The function invoker_isPending() is not used at all. > > > > OK. > > > > > > > > > > > > > > Please, consider your fix reviewed. > > > > Thanks! > > > > > > > > The hg exported changeset is here: > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/JDK-8153711-jdk9-jdk.export.patch > > > > > > > > > > > > > > > > I'd need somebody to sponsor this for me. > > > I will sponsor the fix for you. > > Hi Severin, > > > > I postpone a push for this fix. > > > > There are two nsk.jdi test failures (they look like deadlocks): > > nsk/jdi/ObjectReference/invokeMethod/invokemethod012 FAIL(TIMEOUT) > > nsk/jdi/Scenarios/invokeMethod/popframes001 FAIL(TIMEOUT) Those seem closed tests, no? At least I don't see those. Any pointers? > > and one jtreg sun/com/jdi failure (it looks like a deadlock too): > > com/sun/jdi/InvokeHangTes.java This seems one I can work with. On which platform does it fail? I've run this test > 100 times when I last tested the patch and didn't observe hangs. > > I'll double check if these failures are regressions caused by your fix > > or not. > I confirm, the failures above are new regressions introduced by the fix. > The tests fail consistently with the fix and do not fail without the fix. > This new fix grabs relevant locks for both cases above. Thanks for checking! I'll have another look. Hopefully I can reproduce. Cheers, Severin > > > > > > > > > > > > > > > > Testing done: > > > > > > > > - com/sun/jdi test set. No regressions + added regression > > > > > > > > test. > > > > > > > > - com/sun/jdi/InterfaceMethodsTest.java did not fail in 1300 > > > > > > > > invocations. Same for com/sun/jdi/InvokeTest.java. > > > > > > > > - I haven't seen any crashes in new OomDebugTest.java either. > > > > > > > > > > > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8153711 > > > > > > > > webrev: > >
Re: RFR: 8154728: JvmtiExport::add_default_read_edges hits a guarantee
Thanks Stefan! And for sponsoring! /Robbin On 04/20/2016 10:00 AM, Stefan Karlsson wrote: Hi Robbin, This looks good to me. This breaks gc nightly and is a fairly trivial change, so I'll push this right away. Thanks, StefanK On 2016-04-20 09:40, Robbin Ehn wrote: Hi all, Please review. We are missing a cr on a log stream, which leads to this guarantee. Bug: https://bugs.openjdk.java.net/browse/JDK-8154728 Thanks ! /Robbin diff -r 857efca82258 src/share/vm/prims/jvmtiExport.cpp --- a/src/share/vm/prims/jvmtiExport.cppSun Apr 17 19:15:52 2016 -0700 +++ b/src/share/vm/prims/jvmtiExport.cppWed Apr 20 09:37:01 2016 +0200 @@ -428,6 +428,7 @@ LogTarget(Trace, jvmti) log; LogStreamCHeap log_stream(log); java_lang_Throwable::print(PENDING_EXCEPTION, _stream); +log_stream.cr(); CLEAR_PENDING_EXCEPTION; return; }
Re: RFR: 8154728: JvmtiExport::add_default_read_edges hits a guarantee
Thanks David! /Robbin On 04/20/2016 09:57 AM, David Holmes wrote: Looks fine to me. Thanks, David On 20/04/2016 5:40 PM, Robbin Ehn wrote: Hi all, Please review. We are missing a cr on a log stream, which leads to this guarantee. Bug: https://bugs.openjdk.java.net/browse/JDK-8154728 Thanks ! /Robbin diff -r 857efca82258 src/share/vm/prims/jvmtiExport.cpp --- a/src/share/vm/prims/jvmtiExport.cppSun Apr 17 19:15:52 2016 -0700 +++ b/src/share/vm/prims/jvmtiExport.cppWed Apr 20 09:37:01 2016 +0200 @@ -428,6 +428,7 @@ LogTarget(Trace, jvmti) log; LogStreamCHeap log_stream(log); java_lang_Throwable::print(PENDING_EXCEPTION, _stream); +log_stream.cr(); CLEAR_PENDING_EXCEPTION; return; }
Re: RFR(S): JDK-8153278 sun/tools/jps/TestJpsJar.java fails in hs nightly
David, The problem is observed under Aurora/Windows where the test starts as C:\ but when JPS is run the path is E:\ So the test is failed. I have no ideas how it happens. -Dmitry On 2016-04-20 05:37, David Holmes wrote: > Hi Dmitry, > > On 19/04/2016 11:06 PM, Dmitry Samersoff wrote: >> Everybody, >> >> Please review the changes. >> >> http://cr.openjdk.java.net/~dsamersoff/JDK-8153278/webrev.01/ >> >> The test (a) get it's own name then (b) launch JPS and check that it's >> name is present in the output. >> >> If for some reason the directory from where the test is run changes it >> name between (a) and (b) test fails. >> >> So test changed to get it's name right before the check of jps output. > > Changes seem fine, but I am curious how the directory can be changed > between (a) and (b) ?? > > Thanks, > David > >> -Dmitry >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR: 8154728: JvmtiExport::add_default_read_edges hits a guarantee
Hi Robbin, This looks good to me. This breaks gc nightly and is a fairly trivial change, so I'll push this right away. Thanks, StefanK On 2016-04-20 09:40, Robbin Ehn wrote: Hi all, Please review. We are missing a cr on a log stream, which leads to this guarantee. Bug: https://bugs.openjdk.java.net/browse/JDK-8154728 Thanks ! /Robbin diff -r 857efca82258 src/share/vm/prims/jvmtiExport.cpp --- a/src/share/vm/prims/jvmtiExport.cppSun Apr 17 19:15:52 2016 -0700 +++ b/src/share/vm/prims/jvmtiExport.cppWed Apr 20 09:37:01 2016 +0200 @@ -428,6 +428,7 @@ LogTarget(Trace, jvmti) log; LogStreamCHeap log_stream(log); java_lang_Throwable::print(PENDING_EXCEPTION, _stream); +log_stream.cr(); CLEAR_PENDING_EXCEPTION; return; }
Re: RFR: 8154728: JvmtiExport::add_default_read_edges hits a guarantee
Looks fine to me. Thanks, David On 20/04/2016 5:40 PM, Robbin Ehn wrote: Hi all, Please review. We are missing a cr on a log stream, which leads to this guarantee. Bug: https://bugs.openjdk.java.net/browse/JDK-8154728 Thanks ! /Robbin diff -r 857efca82258 src/share/vm/prims/jvmtiExport.cpp --- a/src/share/vm/prims/jvmtiExport.cppSun Apr 17 19:15:52 2016 -0700 +++ b/src/share/vm/prims/jvmtiExport.cppWed Apr 20 09:37:01 2016 +0200 @@ -428,6 +428,7 @@ LogTarget(Trace, jvmti) log; LogStreamCHeap log_stream(log); java_lang_Throwable::print(PENDING_EXCEPTION, _stream); +log_stream.cr(); CLEAR_PENDING_EXCEPTION; return; }
Re: RFR: 8154728: JvmtiExport::add_default_read_edges hits a guarantee
Thanks Mikael! /Robbin On 04/20/2016 09:51 AM, Mikael Gerdin wrote: Hi Robbin, On 2016-04-20 09:40, Robbin Ehn wrote: Hi all, Please review. We are missing a cr on a log stream, which leads to this guarantee. Bug: https://bugs.openjdk.java.net/browse/JDK-8154728 Thanks ! /Robbin diff -r 857efca82258 src/share/vm/prims/jvmtiExport.cpp --- a/src/share/vm/prims/jvmtiExport.cppSun Apr 17 19:15:52 2016 -0700 +++ b/src/share/vm/prims/jvmtiExport.cppWed Apr 20 09:37:01 2016 +0200 @@ -428,6 +428,7 @@ LogTarget(Trace, jvmti) log; LogStreamCHeap log_stream(log); java_lang_Throwable::print(PENDING_EXCEPTION, _stream); +log_stream.cr(); CLEAR_PENDING_EXCEPTION; return; } Looks good. /Mikael
Re: RFR: 8154728: JvmtiExport::add_default_read_edges hits a guarantee
Hi Robbin, On 2016-04-20 09:40, Robbin Ehn wrote: Hi all, Please review. We are missing a cr on a log stream, which leads to this guarantee. Bug: https://bugs.openjdk.java.net/browse/JDK-8154728 Thanks ! /Robbin diff -r 857efca82258 src/share/vm/prims/jvmtiExport.cpp --- a/src/share/vm/prims/jvmtiExport.cppSun Apr 17 19:15:52 2016 -0700 +++ b/src/share/vm/prims/jvmtiExport.cppWed Apr 20 09:37:01 2016 +0200 @@ -428,6 +428,7 @@ LogTarget(Trace, jvmti) log; LogStreamCHeap log_stream(log); java_lang_Throwable::print(PENDING_EXCEPTION, _stream); +log_stream.cr(); CLEAR_PENDING_EXCEPTION; return; } Looks good. /Mikael
RFR: 8154728: JvmtiExport::add_default_read_edges hits a guarantee
Hi all, Please review. We are missing a cr on a log stream, which leads to this guarantee. Bug: https://bugs.openjdk.java.net/browse/JDK-8154728 Thanks ! /Robbin diff -r 857efca82258 src/share/vm/prims/jvmtiExport.cpp --- a/src/share/vm/prims/jvmtiExport.cppSun Apr 17 19:15:52 2016 -0700 +++ b/src/share/vm/prims/jvmtiExport.cppWed Apr 20 09:37:01 2016 +0200 @@ -428,6 +428,7 @@ LogTarget(Trace, jvmti) log; LogStreamCHeap log_stream(log); java_lang_Throwable::print(PENDING_EXCEPTION, _stream); +log_stream.cr(); CLEAR_PENDING_EXCEPTION; return; }
Re: PING: RFR: JDK-8153073: UL: Set filesize option with k/m/g
On 20/04/2016 3:25 PM, Yasumasa Suenaga wrote:
Hi David,
> You still removed the size bounds checks:
>
> 190 size_t value = parse_value(value_str);
> 191 if (value == SIZE_MAX || value > SIZE_MAX / K) {
> 192 errstream->print_cr("Invalid option: %s must be in range
[0, "
> 193 SIZE_FORMAT "]", FileSizeOptionKey,
SIZE_MAX / K);
> 194 success = false;
SIZE_MAX is defined as ULONG_MAX in stdint.h [1].
Ah I hadn't realized this was an external value, I thought it was some
internally enforced maximum file size limit. So this is just an overflow
check really, and ...
Arguments::atojulong(atomull) checks value range [2].
... we already do an overflow check in here, but ...
Thus I do not think we do not need to check value range in
LogFileOutput::parse_options().
... on 32-bit size_t and julong are not the same size so we would still
need to ensure we don't specify a filesize that is greater than SIZE_MAX
on 32-bit.
> Thanks, I had missed that example usage buried in there, but am still
> surprised none of these "options" for the handling the file are
> explicitly documented.
I do not know how we can documented about it.
(Is it internal process in Oracle?)
No I just meant that amongst all that help text you already modified,
there is nothing, that I could see, that actually describes the possible
options for filesize.
Thanks,
David
I can help for it if I can
Thanks,
Yasumasa
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/generic/stdint.h;h=442762728b899aa8ec219299692fce5953d796b0;hb=HEAD#l259
[2]
http://hg.openjdk.java.net/jdk9/hs/hotspot/file/8005261869c9/src/share/vm/runtime/arguments.cpp#l804
2016/04/20 11:24 "David Holmes" >:
Hi Yasumasa,
On 19/04/2016 11:50 PM, Yasumasa Suenaga wrote:
> Hi David,
>
> Thank you for your comment.
>
> I uploaded new webrev. Could you review again?
> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.02/
You still removed the size bounds checks:
190 size_t value = parse_value(value_str);
191 if (value == SIZE_MAX || value > SIZE_MAX / K) {
192 errstream->print_cr("Invalid option: %s must be in
range [0, "
193 SIZE_FORMAT "]", FileSizeOptionKey,
SIZE_MAX / K);
194 success = false;
>> Which makes me wonder if atomull needs renaming - does the "m" mean
>> memory? atojulong would seem more appropriate regardless.
>
> I renamed to atojulong() in new webrev.
>
>> Not directly related to your change but I was surprised that the
>> various log file options don't seem to be documented anywhere in the
>> -Xlog:help output.
>
> I updated help message in new webrev.
Thanks, I had missed that example usage buried in there, but am still
surprised none of these "options" for the handling the file are
explicitly documented.
Thanks,
David
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/04/19 10:14, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 19/04/2016 12:06 AM, Yasumasa Suenaga wrote:
>>> PING:
>>>
>>> I've sent review request for JDK-8153073.
>>> Could you review it?
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153073/webrev.01/
>>>
>>> If this patch is merged, user can set logfile size with k/m/g.
>>
>> Your webrev seems out of date with respect to the current code - the
>> logfile size processing is done in LogFileOutput::parse_options not
>> configure_rotation. And of course you now need to work with
jdk9/hs not
>> hs-rt.
>>
>> That aside:
>>
>> src/share/vm/runtime/arguments.cpp
>>
>> I don't think you need to add the Arguments:: to the atomull
calls when
>> you are executing in Arguments code - lines 2643, 2660
>>
>> This comment could be updated to delete "memory"
>>
>>788 // Parses a memory size specification string.
>>
>> Which makes me wonder if atomull needs renaming - does the "m" mean
>> memory? atojulong would seem more appropriate regardless.
>>
>> ---
>>
>> src/share/vm/logging/logFileOutput.cpp
>>
>> You removed the size checking logic.
>>
>> Not directly related to your change but I was surprised that the
>> various log file options don't seem to be documented anywhere in the
>> -Xlog:help output.
>>
>> Thanks,
>> David
>> -
>>
>>>
>>> Please review it.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/04/11 18:28, Yasumasa Suenaga wrote:
PING: Could you review it?
We need more reviewer.
>>
