RE: RFR(XS): 8068778: [TESTBUG] CompressedClassSpaceSizeInJmapHeap.java fails if SA not available
Hi, could please somebody from servicability sponsor this tiny change? Thanks, Goetz. -Original Message- From: hotspot-dev [mailto:hotspot-dev-boun...@openjdk.java.net] On Behalf Of Coleen Phillimore Sent: Freitag, 16. Januar 2015 23:56 To: hotspot-...@openjdk.java.net Subject: Re: RFR(XS): 8068778: [TESTBUG] CompressedClassSpaceSizeInJmapHeap.java fails if SA not available This seems fine. Someone from the serviceability group should probably sponsor. Thanks, Coleen On 1/16/15, 6:15 AM, Lindenmaier, Goetz wrote: Hi, could I please get a review for this small fix? I also please need a sponsor. Thanks, Goetz. From: goetz.lindenma...@sap.com Sent: Montag, 12. Januar 2015 09:23 To: hotspot-...@openjdk.java.net Subject: RFR(XS): 8068778: [TESTBUG] CompressedClassSpaceSizeInJmapHeap.java fails if SA not available Hi, This test uses SA, thus fails on platforms where that's not implemented. I see problems on mac and aix. Call Platform.shouldSAAttach() to check whether SA should work. Please review this small fix. I please need a sponsor. http://cr.openjdk.java.net/~goetz/webrevs/8068778-jtregSA/webrev.01/ Best regards, Goetz.
Re: RFR(XXS): 8072472: serviceability/dcmd/framework/* should be quarantined
Thanks for the review. Would you help me push this (exported changeset attached)? Thanks, Mikael On 2015-02-04 17:56, Jaroslav Bachorik wrote: Looks good! -JB- On 4.2.2015 16:13, Mikael Auno wrote: Hi, could I have a review for this small change to ignore three tests. Issue: https://bugs.openjdk.java.net/browse/JDK-8072472 Webrev: http://cr.openjdk.java.net/~miauno/8072472/webrev.00/ Thanks, Mikael # HG changeset patch # User miauno # Date 1423062589 -3600 # Node ID c1552a4dfc143973516a730a0dc87e25ca946eb9 # Parent 79f4205419d2118a7a5a548fd24ca4a82a50460b 8072472: serviceability/dcmd/framework/* should be quarantined Reviewed-by: jbachorik diff --git a/test/serviceability/dcmd/framework/HelpTest.java b/test/serviceability/dcmd/framework/HelpTest.java --- a/test/serviceability/dcmd/framework/HelpTest.java +++ b/test/serviceability/dcmd/framework/HelpTest.java @@ -33,6 +33,7 @@ * @test * @summary Test of diagnostic command help (tests all DCMD executors) * @library /testlibrary + * @ignore 8072440 * @build com.oracle.java.testlibrary.* * @build com.oracle.java.testlibrary.dcmd.* * @run testng/othervm -XX:+UsePerfData HelpTest diff --git a/test/serviceability/dcmd/framework/InvalidCommandTest.java b/test/serviceability/dcmd/framework/InvalidCommandTest.java --- a/test/serviceability/dcmd/framework/InvalidCommandTest.java +++ b/test/serviceability/dcmd/framework/InvalidCommandTest.java @@ -33,6 +33,7 @@ * @test * @summary Test of invalid diagnostic command (tests all DCMD executors) * @library /testlibrary + * @ignore 8072440 * @build com.oracle.java.testlibrary.* * @build com.oracle.java.testlibrary.dcmd.* * @run testng/othervm -XX:+UsePerfData InvalidCommandTest diff --git a/test/serviceability/dcmd/framework/VMVersionTest.java b/test/serviceability/dcmd/framework/VMVersionTest.java --- a/test/serviceability/dcmd/framework/VMVersionTest.java +++ b/test/serviceability/dcmd/framework/VMVersionTest.java @@ -34,6 +34,7 @@ * @test * @summary Test of diagnostic command VM.version (tests all DCMD executors) * @library /testlibrary + * @ignore 8072440 * @build com.oracle.java.testlibrary.* * @build com.oracle.java.testlibrary.dcmd.* * @run testng/othervm -XX:+UsePerfData VMVersionTest
RFR 8067447: Factor out the shared implementation of the VM flags manipulation code
Hi, this is a round 2 review of the proposed solution. Issue : https://bugs.openjdk.java.net/browse/JDK-8067447 Webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.02 This patch is a precursor for implementing https://bugs.openjdk.java.net/browse/JDK-8054890 which itself is a part of JEP-228 (https://bugs.openjdk.java.net/browse/JDK-8043764). Here, the code related to manipulating JVM flags is extracted to a separate WritableFlags class and the codebease is adjusted to use this class. I didn't touch the original (nonstandard) handling of the DTrace specific flags in the Solaris specific attachListener.cpp implementation to keep the change simple and focused. Thanks, -JB-
Re: Fwd: Patch: Clean up fix for JDK-8047720
Dear David,
I took a look at all occurrences of PeriodicTask_lock. We are in luck:
PeriodicTask::enroll and PeriodicTask::disenroll check for safepoints when
acquiring PeriodicTask_lock.
PeriodicTask::time_to_wait and PeriodicTask::real_time_tick are called only
by the watcher thread.
WatcherThread::unpark is used only in contexts where PeriodicTask_lock is
owned by the caller.
WatcherThread::sleep is called only by the watcher thread.
I took another look at WatcherThread::sleep and realized that there is a
path from acquiring PeriodicTask_lock to waiting on the lock where
_should_terminate is not checked. I added that to the minimal fix attached.
Looking at these methods made me want to clean up a little more. See
better.patch attached.
I feel pretty good about the patch with the limited usage of
no_safepoint_check_flag with PeriodicTask_lock.
Carsten
On Tue, Feb 3, 2015 at 11:28 PM, David Holmes david.hol...@oracle.com
wrote:
On 4/02/2015 1:28 PM, Carsten Varming wrote:
Dear David Holmes,
Please see inline response,
On Tue, Feb 3, 2015 at 9:38 PM, David Holmes david.hol...@oracle.com
mailto:david.hol...@oracle.com wrote:
On 4/02/2015 5:00 AM, Carsten Varming wrote:
Greetings all,
I was recently introduced to the deadlock described in
JDK-8047720 and
fixed in JDK9. The fix seems a little messy to me, and it looks
like it
left some very short races in the code. So I decided to clean up
the
code. See attached diff.
The change takes a step back and acquires PeriodicTask_lock in
WatcherThread::stop (like before the fix in JDK9), but this time
safepoints are allowed to proceed when acquiring
PeriodicTask_lock,
preventing the deadlock.
It isn't obvious that blocking for a safepoint whilst in this method
will always be a safe thing to do. That would need to be examined in
detail - potential interactions can be very subtle.
Absolutely true. For reference, the pattern is repeated with the
Terminator_lock a few lines below. The pattern is also used in
Threads::destroy_vm before and after calling before_exit, and the java
shutdown hooks are called right before the call to before_exit. So there
is a lot of evidence that safepoints are allowed to happen in this
context.
The thread calling before_exit is a JavaThread so of course it
participates in safepoints. The issue is whether the interaction between
that thread and the WatcherThread, via the PeriodicTask_lock, can allow for
the JavaThread blocking at a safepoint whilst owning that lock. If another
JavaThread can try to lock it without a safepoint check then we can get a
deadlock.
Cheers,
David
Thank you for taking your time to look at this,
Carsten
David
Let me know what you think,
Carsten
diff --git a/src/share/vm/runtime/task.cpp b/src/share/vm/runtime/task.cpp
--- a/src/share/vm/runtime/task.cpp
+++ b/src/share/vm/runtime/task.cpp
@@ -74,8 +74,7 @@
}
int PeriodicTask::time_to_wait() {
- MutexLockerEx ml(PeriodicTask_lock-owned_by_self() ?
- NULL : PeriodicTask_lock, Mutex::_no_safepoint_check_flag);
+ assert(PeriodicTask_lock-owned_by_self(), precondition);
if (_num_tasks == 0) {
return 0; // sleep until shutdown or a task is enrolled
diff --git a/src/share/vm/runtime/thread.cpp b/src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp
+++ b/src/share/vm/runtime/thread.cpp
@@ -1198,6 +1198,10 @@
int WatcherThread::sleep() const {
MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag);
+ if (_should_terminate) {
+return 0; // we did not sleep.
+ }
+
// remaining will be zero if there are no tasks,
// causing the WatcherThread to sleep until a task is
// enrolled
@@ -1252,7 +1256,7 @@
this-initialize_thread_local_storage();
this-set_native_thread_name(this-name());
this-set_active_handles(JNIHandleBlock::allocate_block());
- while (!_should_terminate) {
+ while (true) {
assert(watcher_thread() == Thread::current(), thread consistency check);
assert(watcher_thread() == this, thread consistency check);
@@ -1288,6 +1292,10 @@
}
}
+if (_should_terminate) {
+ break;
+}
+
PeriodicTask::real_time_tick(time_waited);
}
@@ -1318,24 +1326,20 @@
}
void WatcherThread::stop() {
- // Get the PeriodicTask_lock if we can. If we cannot, then the
- // WatcherThread is using it and we don't want to block on that lock
- // here because that might cause a safepoint deadlock depending on
- // what the current WatcherThread tasks are doing.
- bool have_lock = PeriodicTask_lock-try_lock();
-
_should_terminate = true;
- OrderAccess::fence(); // ensure WatcherThread sees update in main loop
-
- if (have_lock) {
+ {
+// Let safepoints happen to avoid the deadlock:
+// The VM thread has Threads_lock and waits for Java threads
Re: RFR(S): 8071999: SA's buildreplayjars fail with exception
Since you’re saying it only fixes it partially should we file a followup bug and maybe leave a comment behind? On Feb 4, 2015, at 7:04 AM, Roland Westrelin roland.westre...@oracle.com wrote: buildreplayjars sometimes fail with an exception and I think that's caused by the lack of support for default methods when the SA dumps classes. This change fixes it at least partially. It may not be the cleanest way to support default methods but it already proved useful when investigating a compiler crash so unless someone has a better fix I would like to push it. http://cr.openjdk.java.net/~roland/8071999/webrev.00/ Roland.
Re: RewriteFrequentPairs flag was disabled in debug model(and discussion for fix)
Hi,
Just add a few more words to explain the benefit on debug experience...
If with that fix(hmmm, I just assumed that the fix in previous discussion
already covered all the cases to make the RewriteFrequentPairs work in
debug mode)
we at least give the user another option to enable the
RewriteFrequentPairs flag in debug mode, just like we did it for
RewriteBytecodes flag, so that the debugger can also inspect these super
instructions in interpreter in debug mode.
your options?
Thanks!
San Hong
On Tue, Jan 27, 2015 at 9:56 PM, Daniel D. Daugherty
daniel.daughe...@oracle.com wrote:
Resending to include the Serviceability team since they own JVM/TI...
Dan
On 1/26/15 11:53 PM, San Hong Li wrote:
HI All:
When the JVM is running in debug model, the RewriteFrequentPairs
optimization will be disabled by following code in
JvmtiManageCapabilities::update():
* if (avail.can_generate_breakpoint_events) {*
*RewriteFrequentPairs = false;*
* }*
*[ pls. check the above code @Line 328 in jvmtiManageCapabilities.cpp in
jdk7 codebase]*
I understand the reason why the RewriteFrequentPairs doesn't work in
debug
model is that the TOS cache will be disturbed after the execution of
TemplateTable::aload_0 in such this case,
Let us discuss it in more details about what will happen if we enable
RewriteFrequentPairs flag in debugging model...
The following code was excerpted from TemplateTable::aload_0
( @L789 in templateTable_x86_64.cpp).
For better discussion, I just skipped irrelevant parts:
*void TemplateTable::aload_0() {*
* transition(vtos, atos);*
* if (RewriteFrequentPairs) {*
* .. *
* // do actual aload_0*
* aload(0); *
* ..*
* // rewrite*
* // bc: fast bytecode*
* __ bind(rewrite);*
* patch_bytecode(Bytecodes::_aload_0, bc, rbx, false);*
* __ bind(done);*
* } else {*
*aload(0);*
* }*
*}*
In above code:
The *aload(0) *happened before * patch_bytecode. *
The TOS cache will be set correctly after *aload(0) *, that's, the state
should be atos as we have expected.
But the cache will be corrupted by the following *patch_bytecode
*which actually
calls out to InterpreterRuntime::set_original_bytecode_at:
Pls. check the implementation around at L256 in templateTable_x86_64.cpp:
*// Let breakpoint table handling rewrite to quicker bytecode*
*__ call_VM(noreg, CAST_FROM_FN_PTR(address,
InterpreterRuntime::set_original_bytecode_at), temp_reg, r13, bc_reg);*
The problem here is:
The call out into InterpreterRuntime::set_original_bytecode_at will
eventually change the values cached in RAX.
I would like to discuss and propose a fix for this issue: just do
the * aload(0)
*after *patch_bytecode!*
(actually similar thing we did in TemplateTable::iload) ,
So the updated code with fix likes this:
*void TemplateTable::aload_0() {*
* transition(vtos, atos);*
* if (RewriteFrequentPairs) {*
* .. *
* // do actual aload_0*
* //aload(0); *
* ..*
* // rewrite*
* // bc: fast bytecode*
* __ bind(rewrite);*
* patch_bytecode(Bytecodes::_aload_0, bc, rbx, false);*
* __ bind(done);*
* } *
* // do actual aload_0*
*__ movptr(rax, aaddress(0));*
*}*
So that the TOS has been cached correctly without disruption by
*patch_bytecode,
*safe use for next bytecode.
Finally, just a summary for my questions:
Whether or not *this is the only case t*hat explained why the current
implementation of HotSpot has to disable RewriteFrequentPairs flag in
debug mode?
--- If so, how do u think about the fix? I did some tests in my
environment, it works fine.
--- if not, would you pls. point out in which case we also have to
disable RewriteFrequentPairs during debugging?
Thanks!
San Hong
Re: RFR(S): 8071999: SA's buildreplayjars fail with exception
On 04.02.2015 17:16, Christian Thalinger wrote: Since you’re saying it only fixes it partially should we file a followup bug and maybe leave a comment behind? I think this one is related: https://bugs.openjdk.java.net/browse/JDK-8015848 Best, Tobias On Feb 4, 2015, at 7:04 AM, Roland Westrelin roland.westre...@oracle.com wrote: buildreplayjars sometimes fail with an exception and I think that's caused by the lack of support for default methods when the SA dumps classes. This change fixes it at least partially. It may not be the cleanest way to support default methods but it already proved useful when investigating a compiler crash so unless someone has a better fix I would like to push it. http://cr.openjdk.java.net/~roland/8071999/webrev.00/ Roland.
Re: RFR: JDK-8068589: GCCause should distinguish jcmd GC.run from System.gc()
Looks good. -Tao (tamao) On Mon, Feb 2, 2015 at 7:51 AM, Yasumasa Suenaga yasue...@gmail.com wrote: Hi, I need more reviewer. Could you review it? http://cr.openjdk.java.net/~ysuenaga/JDK-8068589/webrev.02/ Thanks, Yasumasa 2015/01/28 17:24 Staffan Larsen staffan.lar...@oracle.com: Looks good! Thanks, /Staffan On 28 jan 2015, at 05:48, Yasumasa Suenaga yasue...@gmail.com wrote: Hi Staffan, Kirk, I agree to set Diagnostic Command to GCCause. So I applied it to new patch. http://cr.openjdk.java.net/~ysuenaga/JDK-8068589/webrev.02/ Could you review it again? Thanks, Yasumasa On 2015/01/28 5:06, Kirk Pepperdine wrote: Hi Staffan, Anyway, it’s a record in a GC log so I don’t see the value of GC.run. Certainly “DiagCmd or even Diagnostic Command” seems sufficient given the context. Let’s go with “Diagnostic Command”, then. Thank you! Regards, Kirk
Re: RFR(S, testonly): JDK-8072395: LingeredAppTest.java and JMapHeapConfigTest.java fail due to LingeredApp ERROR: java.io.IOException: Lock is too old. Aborting
Staffan, Are you OK to push the changes? -Dmitry On 2015-02-03 16:01, Dmitry Samersoff wrote: Everyone, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8072395/webrev.01/ 1. I was wrong in my assumption that the test can't run more than an hour of physical machine time when wrote the test. This check is removed. 2. Added sanity check for common environment pitfalls to have better diagnostic in case of a fail 3. The test kept disabled on Mac OS X until the problem with attach permissions for non-root users is solved. -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(XS): 8068778: [TESTBUG] CompressedClassSpaceSizeInJmapHeap.java fails if SA not available
This is obsolete, it stuck in the queue waiting moderator approval. Sorry, Goetz. -Original Message- From: serviceability-dev [mailto:serviceability-dev-boun...@openjdk.java.net] On Behalf Of Lindenmaier, Goetz Sent: Wednesday, January 21, 2015 11:40 AM To: hotspot-...@openjdk.java.net; serviceability-dev Subject: RE: RFR(XS): 8068778: [TESTBUG] CompressedClassSpaceSizeInJmapHeap.java fails if SA not available Hi, could please somebody from servicability sponsor this tiny change? Thanks, Goetz. -Original Message- From: hotspot-dev [mailto:hotspot-dev-boun...@openjdk.java.net] On Behalf Of Coleen Phillimore Sent: Freitag, 16. Januar 2015 23:56 To: hotspot-...@openjdk.java.net Subject: Re: RFR(XS): 8068778: [TESTBUG] CompressedClassSpaceSizeInJmapHeap.java fails if SA not available This seems fine. Someone from the serviceability group should probably sponsor. Thanks, Coleen On 1/16/15, 6:15 AM, Lindenmaier, Goetz wrote: Hi, could I please get a review for this small fix? I also please need a sponsor. Thanks, Goetz. From: goetz.lindenma...@sap.com Sent: Montag, 12. Januar 2015 09:23 To: hotspot-...@openjdk.java.net Subject: RFR(XS): 8068778: [TESTBUG] CompressedClassSpaceSizeInJmapHeap.java fails if SA not available Hi, This test uses SA, thus fails on platforms where that's not implemented. I see problems on mac and aix. Call Platform.shouldSAAttach() to check whether SA should work. Please review this small fix. I please need a sponsor. http://cr.openjdk.java.net/~goetz/webrevs/8068778-jtregSA/webrev.01/ Best regards, Goetz.
Re: RFR(S): 8071999: SA's buildreplayjars fail with exception
Looks good. Best regards, Vladimir Ivanov On 2/4/15 4:04 PM, Roland Westrelin wrote: buildreplayjars sometimes fail with an exception and I think that's caused by the lack of support for default methods when the SA dumps classes. This change fixes it at least partially. It may not be the cleanest way to support default methods but it already proved useful when investigating a compiler crash so unless someone has a better fix I would like to push it. http://cr.openjdk.java.net/~roland/8071999/webrev.00/ Roland.
Re: RFR(S): 8071999: SA's buildreplayjars fail with exception
Hi Roland, On 5/02/2015 1:04 AM, Roland Westrelin wrote: buildreplayjars sometimes fail with an exception and I think that's caused by the lack of support for default methods when the SA dumps classes. This change fixes it at least partially. It may not be the cleanest way to support default methods but it already proved useful when investigating a compiler crash so unless someone has a better fix I would like to push it. http://cr.openjdk.java.net/~roland/8071999/webrev.00/ I wouldn't say it supports default methods rather it ignores them so they don't trigger exceptions. I'm still puzzling over why default methods have a zero _codeIndex as that seems to be the real cause of the trouble. ?? As a quick workaround it may be okay but it appears that SA is seriously lagging in its language support. :( Cheers, David Roland.
RFR(S): 8071999: SA's buildreplayjars fail with exception
buildreplayjars sometimes fail with an exception and I think that's caused by the lack of support for default methods when the SA dumps classes. This change fixes it at least partially. It may not be the cleanest way to support default methods but it already proved useful when investigating a compiler crash so unless someone has a better fix I would like to push it. http://cr.openjdk.java.net/~roland/8071999/webrev.00/ Roland.
Re: RFR(S): 8071999: SA's buildreplayjars fail with exception
Roland, Looks good for me! (not a reviewer) Thank you for doing it. -Dmitry On 2015-02-04 18:04, Roland Westrelin wrote: buildreplayjars sometimes fail with an exception and I think that's caused by the lack of support for default methods when the SA dumps classes. This change fixes it at least partially. It may not be the cleanest way to support default methods but it already proved useful when investigating a compiler crash so unless someone has a better fix I would like to push it. http://cr.openjdk.java.net/~roland/8071999/webrev.00/ Roland. -- 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(XXS): 8072472: serviceability/dcmd/framework/* should be quarantined
Hi, could I have a review for this small change to ignore three tests. Issue: https://bugs.openjdk.java.net/browse/JDK-8072472 Webrev: http://cr.openjdk.java.net/~miauno/8072472/webrev.00/ Thanks, Mikael
