Re: RFR 8067241 DeadlockTest.java failed with negative timeout value
On 12/15/2014 06:38 PM, Daniel Fuchs wrote:
On 12/12/14 09:56, shanliang wrote:
Updated.
Here is the new version:
http://cr.openjdk.java.net/~sjiang/JDK-8067241/01/
Thanks,
Shanliang
Hi Shanliang,
Your changes looks good to me. WRT using a Phaser, it would
require a careful analysis to assert that modifying the
locking strategy - especially things like
90 synchronized(o) {
91 synchronized(this) {
92 gotLock = true;
93
94 this.notify();
95 }
still preserve the nature of the test.
I wonder if it's worth the trouble.
Well, the test tries to achieve step-by-step execution of two concurrent
threads where one thread can only proceed when the other one has already
reached a predefined execution point. This is exactly what the Phaser is
fit for. Please note that by using Phasero one could also get rid off
the Thread.sleep() construct to simulate the fact
that the BadBoy thread doesn't finish before the main thread had the
chance to assert the postconditions.
But the change is already in .. what is done is done. It's just a pity
we missed the chance to improve the test a bit while fixing this issue.
-JB-
best regards,
-- daniel
RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
Hi, Could I please have a review of this fix. bug: https://bugs.openjdk.java.net/browse/JDK-6977426 webrev: http://cr.openjdk.java.net/~ykantser/6977426/webrev.00/ java/util/logging/LoggerWeakRefLeak.sh and java/util/logging/AnonLoggerWeakRefLeak.sh are merged and rewritten in java. sun/tools/common/CommonTests.sh is removed because it doesn't test the product but sun/tool/common library functionality. Thanks, Katja
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
Hi Katja,
This request should probably go to core-libs as well.
Other than that I have just a few nits:
test/java/util/logging/TestLoggerWeakRefLeak.java
L57: if ("Logger".contains(args[0])) -> "Logger".equals(args[0]) ?
L127: // in LogManager.loggers that *arebeing* properly managed -> *are
being*
callLoggerAndCount() and callAnonymousLoggerAndCount() are using the
'count' variable to always return 100. Could they be made to return a
constant instead?
Cheers,
-JB-
On 12/16/2014 04:57 PM, Yekaterina Kantserova wrote:
Hi,
Could I please have a review of this fix.
bug: https://bugs.openjdk.java.net/browse/JDK-6977426
webrev: http://cr.openjdk.java.net/~ykantser/6977426/webrev.00/
java/util/logging/LoggerWeakRefLeak.sh and
java/util/logging/AnonLoggerWeakRefLeak.sh are merged and rewritten in
java. sun/tools/common/CommonTests.sh is removed because it doesn't test
the product but sun/tool/common library functionality.
Thanks,
Katja
Re: RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
On 12/15/2014 08:32 AM, Dmitry Samersoff wrote:
> Volker,
>
> This is part two of review. Code looks good for me, only minor nits.
Thanks for the review, Dmitry.
*Volker*, will you be making the suggested changes (from all review comments)
to the patch
set or do you want me to do it? Actually, I am retiring from IBM and, as of
Friday, will no
longer have access to a ppc64 machine or to my IBM email account. So my
colleague,
Tiago Daitx (on cc) will take over for me any responsibilities left in pushing
this patch set
upstream.
Thanks again for all the help.
-Maynard
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64CFrame.java:
>
> 41 missed space around =
> 51 extra space between pc
>
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64ThreadContext.java
>
>
> - no comments
>
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/ppc64/PPC64ThreadContext.java
>
> 47, 48 extra space before =
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64Thread.java
>
> 34 extra space before id
> 42 extra space before = ,
>it might be better to create a constant for size of integer.
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadContext.java
>
> - no comments
>
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadFactory.java
>
> -no comments
>
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64Thread.java
>
> 43 missed space before ?
>it might be be better to reformat this line to usual if and add comments
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadContext.java
>
> - no comments
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadFactory.java
>
> - no comments
>
> *
> agent/src/share/classes/sun/jvm/hotspot/runtime/linux_ppc64/LinuxPPC64JavaThreadPDAccess.java
>
>
> 57, 58 extra space before =
>
> 63 and below extra space after public
>
> 108 unaligned comments
>
> *
> agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64CurrentFrameGuess.java
>
> 49 Formatting in PPC64Frame.java looks much better for me.
>
> 40 private static final boolean DEBUG;
> 41 static {
> 42 DEBUG = ...
> 43 }
>
>
> 60, 61 extra space before =
>
> 147 missed {} after if (DEBUG)
>
> * agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64Frame.java
>
> 49 - 61, please fix extra and missed spaces
>
> 64,67 - extra spaces after static
> 81 missed space after (int)
>
> 115,137 - I would prefer to always use {} after if
>
> 212 - multiple missed space before '?'
>
> 271 extra space before return
>
> *
> agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64JavaCallWrapper.java
>
> - no comments
>
> *
> agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64RegisterMap.java
>
> - no comments
>
> -Dmitry
>
>
> On 2014-12-09 21:10, Volker Simonis wrote:
>> Hi,
>>
>> can somebody from the serviceability team please review this webrev?
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/8049716
>> https://bugs.openjdk.java.net/browse/JDK-8049716
>>
>> The shared changes are really all trivial.
>>
>> Thanks,
>> Volker
>>
>>
>> On Fri, Dec 5, 2014 at 4:01 PM, Volker Simonis
>> wrote:
>>> Hi Sasha,
>>>
>>> thanks for looking at this change.
>>> I'll incorporate your suggestions into the final version.
>>> I'm just waiting for one more review before preparing a new webrev.
>>>
>>> Regards,
>>> Volker
>>>
>>>
>>> On Fri, Dec 5, 2014 at 3:10 PM, Maynard Johnson wrote:
On 12/04/2014 07:50 PM, Alexander Smundak wrote:
> You are correct, but there no need to have this code for LE at all.
Agreed. I'm fine with adding the "&& !defined(ABI_ELFv2)" throughout that
file
along with the "#if defined(ppc64)".
>
> BTW, a bit on nitpicking in the same file:
> +String endian = System.getProperty("sun.cpu.endian");
> +if (endian.equals("big"))
> + return true;
> +else
> + return false;
> can be rewirtten as
> return "big".equals(System.getProperty("sun.cpu.endian"));
Right. A silly piece of coding there. :-/
-Maynard
>
>
> On Thu, Dec 4, 2014 at 3:43 PM, Maynard Johnson
> wrote:
>> On 12/04/2014 01:15 PM, Alexander Smundak wrote:
>>> The changes for agent/src/os/linux/symtab.c
>>> b/agent/src/os/linux/symtab.c in
>>> http://cr.openjdk.java.net/~simonis/webrevs/8049716 will break
>>> Linux/PPC64 little-endian:
>>> it uses ABIv2, which dropped function descriptors. So the preprocessor
>>> brackets in it should
>>> read
>>> #if defined(ppc64) && !defined(ABI_ELFv2)
>>> instead of just
>>> #if defined(ppc64)
>> Hi, Alexander,
>> I think this is actually fine everywhere except one place. The 'opd'
>> variable will be
>> set to something other than
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
You could use a lambda instead of a boolean and the duplicated
functions, i.e.
public static void main(String[] args) throws Exception {
if (args[0].equals("Logger")) {
testIfLeaking(TestLoggerWeakRefLeak::getLogger);
} else {
testIfLeaking(TestLoggerWeakRefLeak::getAnonymousLogger);
}
}
private static void testIfLeaking(Runnable getLogger) {
...
getLogger.run();
...
}
Erik
Yekaterina Kantserova skrev 2014-12-16 16:57:
Hi,
Could I please have a review of this fix.
bug: https://bugs.openjdk.java.net/browse/JDK-6977426
webrev: http://cr.openjdk.java.net/~ykantser/6977426/webrev.00/
java/util/logging/LoggerWeakRefLeak.sh and
java/util/logging/AnonLoggerWeakRefLeak.sh are merged and rewritten in
java. sun/tools/common/CommonTests.sh is removed because it doesn't
test the product but sun/tool/common library functionality.
Thanks,
Katja
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
Hi Katja, Nice too see some more shell scripts go away! I wonder - shouldn't the test force a System.gc() at some point before taking the histogram? If not what guarantee do we have that the various weak references will be purged? best regards, -- daniel On 16/12/14 16:57, Yekaterina Kantserova wrote: Hi, Could I please have a review of this fix. bug: https://bugs.openjdk.java.net/browse/JDK-6977426 webrev: http://cr.openjdk.java.net/~ykantser/6977426/webrev.00/ java/util/logging/LoggerWeakRefLeak.sh and java/util/logging/AnonLoggerWeakRefLeak.sh are merged and rewritten in java. sun/tools/common/CommonTests.sh is removed because it doesn't test the product but sun/tool/common library functionality. Thanks, Katja
Re: RFR 8066708: JMXStartStopTest fails to connect to port 38112
Hi Dmitry, Strictly speaking you are correct. As soon as you close a socket, there is a possibility -- perhaps vanishingly small but nonzero -- that you might not be able to open it again. The first scenario, where the user of the socket itself opens the socket using an ephemeral port (e.g. new ServerSocket(0)) is of course preferred. This avoids race conditions entirely. It's the second case that I'm still wrestling with, and maybe Jaroslav too. It's fairly difficult to get such "black box" systems to open an ephemeral port and report it back, as opposed to opening up their service on some port number handed in from the outside. (For RMI, rmid is the culprit here. I don't know about JMX.) What makes this difficult is that the rmid service is running in a separate VM, so getting reliable information back from it can be difficult. It's also fairly difficult to establish the retry logic in such cases. If the service fails with a BindException, maybe -- maybe -- it was because there was a conflict over the port, and a retry is warranted. But this needs to be distinguished from other failure modes that might occur, that should be reported as failures instead of causing a retry. In principle, this is possible to do, of course, it's just that it involves more restructuring of the tests, and possibly adding debug/test code to rmid. (It may yet come to that.) I'm still pondering the reasons that, in the open/close/reopen scenario, why the reopen might fail. The obvious reason is that some other process on the system has opened that port between the close and the reopen. I admit that this is a possibility. However, with the open/close/reopen scenario in place, we see tests that fail up to 15% of the time with BindExceptions. This is an extraordinarily high failure rate to be caused by some random other process happening to open the same port in the few microseconds between the close and reopen. It's simply not believable to me. My thinking is still that the port isn't ready for reuse until a small amount of time after it's closed. I have some test programs that exercise sockets in a particular way (e.g., from multiple threads, or opening and closing batches of sockets) that can reproduce the problem on some systems, and these test programs seem to behave better if a time delay is added between the close and the reopen. The exact circumstances under which the problem occurs is difficult to pin down and seems OS specific, and so choosing the "right" delay time is very difficult. But it does strengthen this conjecture in my mind. Naturally it would be better if there were a way to determine when a port is available for reuse without actually opening it. I'm not aware of any such way, but I'm holding onto a little hope that one can be found. s'marks On 12/11/14 10:18 AM, Dmitry Samersoff wrote: Stuart, As soon as you close socket, you open a door for the race. So you need another communication channel to pass a port number (or bind result) between a client and a server without closing a socket on the server side. Typical scenario used by network related code is: 1. Server opens the socket 2. Server binds to port(0) 3. Server gets port number assigned by OS 4. Server informs client (e.g. write the port down to known file, broadcast it etc) 5. Client establishes connection. If the server is a blackbox and have to get a port number from outside, scenario looks like: WHILE(!success and !timeout) 1. Driver chooses random port number 2. Driver runs a server with this number 3. Driver checks that server is actually listening on this port (e.g. try to connect by it self) WEND 4. Driver runs a client with this port number or bails out with descriptive error message. -Dmitry On 2014-12-11 20:53, Stuart Marks wrote: On 12/11/14 7:09 AM, olivier.lagn...@oracle.com wrote: On 11/12/2014 15:43, Dmitry Samersoff wrote: You can set SO_LINGER to zero, in this case socket will be closed immediately without waiting in TIME_WAIT SO-LINGER did not help either in my case (see my previous mail to Jaroslav). That ended-up in using another hard-coded (supposedly free) port. Note that was before RMI tests used randomly allocated ports. But there are no reliable way to predict whether you can take this port or not after you close it. This is what I observed in my case. So the only valid solution is to try to connect to a random port and if this attempt fails try another random port. Everything else will cause more or less frequent intermittent failures. IIRC think this is what is currently done in RMI tests. The RMI tests are still suffering from this problem, unfortunately. The RMI test library gets a "random" port with "new ServerSocket(0)", gets the port number, closes the socket, then returns the port to the caller. The caller then assumes that it can use that port as it wishes. That's when the BindException can occur. There are about 10 RMI test bugs in the da
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
Hi Daniel,
Thank you for reviewing the fix! That's what 'vm.heapHisto("-live")'
does - it will request a full GC.
Best regards,
Katja
On 12/16/2014 06:37 PM, Daniel Fuchs wrote:
Hi Katja,
Nice too see some more shell scripts go away!
I wonder - shouldn't the test force a System.gc()
at some point before taking the histogram?
If not what guarantee do we have that the various weak references
will be purged?
best regards,
-- daniel
On 16/12/14 16:57, Yekaterina Kantserova wrote:
Hi,
Could I please have a review of this fix.
bug: https://bugs.openjdk.java.net/browse/JDK-6977426
webrev: http://cr.openjdk.java.net/~ykantser/6977426/webrev.00/
java/util/logging/LoggerWeakRefLeak.sh and
java/util/logging/AnonLoggerWeakRefLeak.sh are merged and rewritten in
java. sun/tools/common/CommonTests.sh is removed because it doesn't test
the product but sun/tool/common library functionality.
Thanks,
Katja
