Hi Serguei,

It was intentional, there were two considerations here:
1. Test VirtualMachine/suspend/suspend001.java doesn't include a step that 
specifically expects that timeout should happen and checks for it (test 
ThreadReference/suspend/suspend001.java has such step, line 283)
2. Method breakpoint() in  VirtualMachine/suspend/suspend001.java doesn't 
result in any timeout failure in jtreg or Mach5 builds.  

Best regards,
Daniil


On 2/28/19, 3:06 AM, "[email protected]" <[email protected]> 
wrote:

    Hi Daniil,
    
    The fix looks good.
    
    One comment though.
    Did you miss to fix a timeout and comment in 
    VirtualMachine/suspend/suspend001.java
    for breakpoint() the same way as it is done in 
    ThreadReference/suspend/suspend001.java?
    Or it is intentional?
    
    Thanks,
    Serguei
    
    On 2/27/19 21:51, Daniil Titov wrote:
    > Hi Chris,
    >
    > Please review a new version that has this timeout increased as you 
suggested.
    >
    > Webrev: http://cr.openjdk.java.net/~dtitov/8207367/webrev.05/
    > Bug: https://bugs.openjdk.java.net/browse/JDK-8207367
    >
    > Thanks!
    > --Daniil
    >
    > On 2/27/19, 6:57 PM, "Chris Plummer" <[email protected]> wrote:
    >
    >      Hi Daniil,
    >      
    >      I don't think exclfilter001.java and filter001.java are quite right:
    >      
    >         92         waitTime = argHandler.getWaitTime();
    >        154         long waitTimeout = Utils.adjustTimeout(waitTime * 200);
    >        161             eventSet = 
debugee.VM().eventQueue().remove(waitTimeout);
    >      
    >      This looks like a 1 second wait whereas before I thought it was 5, 
which
    >      I thought was a good time to avoid timeouts from dropped packets. You
    >      also are using 5 seconds for remove() calls in other tests in this
    >      webrev. So I suggest just replacing 200 with 1000,
    >      
    >      The rest of the changes look good.
    >      
    >      thanks,
    >      
    >      Chris
    >      
    >      
    >      On 2/27/19 5:44 PM, Daniil Titov wrote:
    >      > Hi Chris and Serguei,
    >      >
    >      > Please review  the new version of the fix that includes the 
changes Chris suggested.
    >      >
    >      > Webrev: http://cr.openjdk.java.net/~dtitov/8207367/webrev.04
    >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8207367
    >      >
    >      > Thanks!
    >      > --Daniil
    >      >
    >      >
    >      > On 2/27/19, 5:10 PM, "Daniil Titov" <[email protected]> 
wrote:
    >      >
    >      >      Hi Chris,
    >      >
    >      >      >>    Is there a reason not to adjust it by 
argHandler.getWaitTime()?
    >      >      I agree, for consistency it makes sense to adjust it by 
argHandler.getWaitTime().
    >      >
    >      >      Thanks.
    >      >      Daniil
    >      >
    >      >      On 2/27/19, 4:54 PM, "Chris Plummer" 
<[email protected]> wrote:
    >      >
    >      >          On 2/27/19 4:09 PM, Daniil Titov wrote:
    >      >          > Hi Chris,
    >      >          >
    >      >          >> It look like the exclfilter001.java loop will always 
end up looping for  eventWaitTime seconds:
    >      >          > It is true for  revision 3 of the patch 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassUnloadRequest/addClassExclusionFilter/exclfilter001.java.sdiff.html
    >      >          >
    >      >          > But in revision 1 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassUnloadRequest/addClassExclusionFilter/exclfilter001.java.sdiff.html
   the loop could finish earlier if timeout happens on lines 160 or 183 (since 
eventSet is null).
    >      >          > When talking about 4 seconds I was referring to lines 
160, 163 and 183. Now the timeout passed in remove(waitTime) method is 4 
seconds in Mach5. But the total waiting time is limited by 200 seconds. If no 
events are delivered for 4 seconds timespan then the method returns earlier.
    >      >          > It seems as 4 seconds is quite sufficient, but we could 
increase it if there are some concerns here.
    >      >          >
    >      >          > 153         long waitTime = Utils.adjustTimeout(1000);
    >      >          > 160             eventSet = 
debugee.VM().eventQueue().remove(waitTime);
    >      >          > 185                 eventSet = 
debugee.VM().eventQueue().remove(waitTime);
    >      >
    >      >          Is there a reason not to adjust it by 
argHandler.getWaitTime()?
    >      >
    >      >          Chris
    >      >
    >      >          >
    >      >          >
    >      >          > Thanks!
    >      >          >
    >      >          > --Daniil
    >      >          >
    >      >          >
    >      >          > On 2/27/19, 3:08 PM, "Chris Plummer" 
<[email protected]> wrote:
    >      >          >
    >      >          >
    >      >          >
    >      >          >      On 2/27/19 2:33 PM, Daniil Titov wrote:
    >      >          >      > Hi Chris,
    >      >          >      >
    >      >          >      > The change in while loop  in exclfilter001.java 
between the first and the last revisions was to ensure that all events are 
drained before the method returns and the next check starts. It used to wait 
for 5 minutes and now it keeps receiving events in portions waiting for 1 
seconds at every iteration.
    >      >          >      It look like the exclfilter001.java loop will 
always end up looping for
    >      >          >      eventWaitTime seconds:
    >      >          >
    >      >          >         92         eventWaitTime =
    >      >          >      Utils.adjustTimeout(argHandler.getWaitTime() * 
10000);
    >      >          >
    >      >          >        125             receiveEvents(eventWaitTime, 
patterns[i]);
    >      >          >
    >      >          >      I think you said that argHandler.getWaitTime() is 
5 and
    >      >          >      Utils.adjustTimeout() is another 4x for mach5 
jobs, so this would mean a
    >      >          >      total of 200 seconds, and this is how much time 
the loop would always
    >      >          >      take since it loops for eventWaitTime seconds.
    >      >          >      >   You mentioned in one of previous emails that 
due to network issues  it might be the case that for some iteration we fail to 
receive an event within this smaller timeout. But later, if I understood you 
right, you suggested that 4 seconds ( Utils.adjustTimeout(1000) adjusts timeout 
for Mach5 builds to 4 seconds) should be sufficient.
    >      >          >      Actually I was going more by the 
argHandler.getWaitTime() adjustment,
    >      >          >      which I thought was 5. Mach5 will multiply that by 
4. I think 1s is too
    >      >          >      short but 5s is probably ok, and 20s for mach5 
jobs should prevent any
    >      >          >      noise from rare network issues, except for serious 
ones that we can't
    >      >          >      expect to recover from.
    >      >          >      >    I agree that it's better to make the test 
return earlier and I will revert changes in exclfilter001.java and 
filter001.java to the first revision.
    >      >          >      Ok.
    >      >          >      >
    >      >          >      > For consistency I will change line 153 to take 
into account a wait time specified in argHandler.getWaitTime()
    >      >          >      > 153         long waitTime = 
Utils.adjustTimeout(1000);
    >      >          >      >
    >      >          >      Ok.
    >      >          >      > I am sorry I missed you comments abut 
Utils.java. You are right we don’t need a new method and 
Utils.waitForCondition() should be sufficient.
    >      >          >      > There is no need for passing logger into since 
if the thread is interrupted the error is thrown and the test fails.  Will send 
a new webrev as soon as testing completes.
    >      >          >      thanks,
    >      >          >
    >      >          >      Chris
    >      >          >      > Thanks!
    >      >          >      > Daniil
    >      >          >      >
    >      >          >      >
    >      >          >      > On 2/27/19, 12:08 PM, "Chris Plummer" 
<[email protected]> wrote:
    >      >          >      >
    >      >          >      >      Hi Daniil,
    >      >          >      >
    >      >          >      >      In exclfilter001.java, since the first 
revision you changed the while
    >      >          >      >      loop to be "while (true)". I'm not sure of 
the reasoning. It used to
    >      >          >      >      exit the first time remove() didn't return 
an EventSet. Now it retries
    >      >          >      >      until the total waittime is exceeded.
    >      >          >      >
    >      >          >      >      When waiting for an event, sometimes you 
use:
    >      >          >      >
    >      >          >      >         91         eventWaitTime =
    >      >          >      >      
Utils.adjustTimeout(argHandler.getWaitTime() * 10000);
    >      >          >      >
    >      >          >      >      and sometimes:
    >      >          >      >
    >      >          >      >        153         long waitTime = 
Utils.adjustTimeout(1000);
    >      >          >      >
    >      >          >      >      Why the discrepancy. In one case you are 
willing to wait 50 seconds for
    >      >          >      >      an event and in the other only 5.
    >      >          >      >
    >      >          >      >      I think you missed my initial comment on 
Utils.java. See below:
    >      >          >      >
    >      >          >      >      > In Utils.java, I think wait() should be 
moved right after
    >      >          >      >      > waitForCondition() and maybe given a more 
descriptive name. It seems
    >      >          >      >      > to basically the same as 
waitForCondition(), except you added a "log"
    >      >          >      >      > parameter and slightly changed the 
behavior. Are these behavior
    >      >          >      >      > differences necessary? Could you share 
code with the existing
    >      >          >      >      > waitForCondition()?
    >      >          >      >
    >      >          >      >      thanks,
    >      >          >      >
    >      >          >      >      Chris
    >      >          >      >
    >      >          >      >      On 2/27/19 9:03 AM, Daniil Titov wrote:
    >      >          >      >      > Hi Serguei and Chris,
    >      >          >      >      >
    >      >          >      >      > Thank you for reviewing this change. 
Please review a new version of the fix that addresses these findings.
    >      >          >      >      >
    >      >          >      >      > Bug: 
https://bugs.openjdk.java.net/browse/JDK-8207367
    >      >          >      >      > Webrev: 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.03
    >      >          >      >      >
    >      >          >      >      > Best regards,
    >      >          >      >      > Daniil
    >      >          >      >      >
    >      >          >      >      > From: <[email protected]>
    >      >          >      >      > Organization: Oracle Corporation
    >      >          >      >      > Date: Tuesday, February 26, 2019 at 6:50 
PM
    >      >          >      >      > To: Daniil Titov 
<[email protected]>, Chris Plummer <[email protected]>, OpenJDK 
Serviceability <[email protected]>
    >      >          >      >      > Subject: Re: RFR 8207367: 10 
vmTestbase/nsk/jdi tests timed out when running with jtreg
    >      >          >      >      >
    >      >          >      >      > Hi Daniil,
    >      >          >      >      >
    >      >          >      >      > It looks good to me.
    >      >          >      >      > I have some minor comments though.
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassUnloadRequest/addClassFilter/filter001.java.frames.html
    >      >          >      >      >
    >      >          >      >      >   163                 if(eventSet != 
null) {
    >      >          >      >      >   Space is missed after if
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove/remove004.java.frames.html
    >      >          >      >      >    70  * In first one, second thread 
waits for any incoming event from the    <BR>
    >      >          >      >      >    71  * debugger which is sleeping for 
some time; hence, <BR>
    >      >          >      >      >    72  * no events are expected to be 
received at the debugger end.           <BR>
    >      >          >      >      >   <BR> is not aligned at 71
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      >   While you are at this code, could you, 
please,
    >      >          >      >      >   also fix unneeded spaces at the lines? :
    >      >          >      >      >   516             } catch ( 
InterruptedException e1) {
    >      >          >      >      >   ...
    >      >          >      >      >   526             } catch ( Exception e ) 
{
    >      >          >      >      >
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove/remove004a.java.frames.html
    >      >          >      >      >   111                                     
    Utils.adjustTimeout(argHandler.getWaitTime() * 10000),
    >      >          >      >      >   112                                     
   100,
    >      >          >      >      >   113                                     
    remove004a::log1);
    >      >          >      >      >    Line 112 is not properly aligned
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l004.java.frames.html
    >      >          >      >      >    65  * In the first one the first 
assertion is checked up on as follows:    <BR>
    >      >          >      >      >    66  * the debugger sleeps for some 
time;                                  <BR>
    >      >          >      >      >    67  * hence, no event is expected in 
the debugger within WAITTIME, and     <BR>
    >      >          >      >      >    <BR> at 66 is not aligned
    >      >          >      >      >    Thank you for catching and fixing the 
typo at 67!
    >      >          >      >      >
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose002.java.frames.html
    >      >          >      >      >
    >      >          >      >      >   295                         }, 
Utils.adjustTimeout(waitTime * 60000), 1000, dispose002::log3);
    >      >          >      >      >   A separate line is needed for next 
wait() argument
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose002a.java.frames.html
    >      >          >      >      >   128                          
while(true) {
    >      >          >      >      >   ...
    >      >          >      >      >   130                              
if(instruction.equals("check_done")){
    >      >          >      >      >   131                                  if 
(test_thread.isAlive()) {
    >      >          >      >      >   132                                     
 exitCode = FAILED;
    >      >          >      >      >   133                                  }
    >      >          >      >      >
    >      >          >      >      >    Space is missed after while.
    >      >          >      >      >    Some logErr("...") for failed case 
would be useful.
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose003.java.frames.html
    >      >          >      >      >   260                         }, 
Utils.adjustTimeout(waitTime * 60000), 1000, dispose003::log3);
    >      >          >      >      >   New line is needed for next wait() 
argument
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose003a.java.frames.html
    >      >          >      >      >   129                          
while(true) {
    >      >          >      >      >   130                              
instruction = pipe.readln();
    >      >          >      >      >   131                              
if(instruction.equals("check_done")) {
    >      >          >      >      >   132                                  if 
(test_thread.isAlive()) {
    >      >          >      >      >   133                                     
 exitCode = FAILED;
    >      >          >      >      >   134                                  }
    >      >          >      >      >    Space is missed after while and if.
    >      >          >      >      >    Some logErr("...") for failed case 
would be useful.
    >      >          >      >      >
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose004.java.frames.html
    >      >          >      >      >   284                         }, 
Utils.adjustTimeout(waitTime * 60000), 1000, dispose004::log3);
    >      >          >      >      >   New line is needed for next wait() 
argument
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      > 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose004a.java.frames.html
    >      >          >      >      >   130                          
while(true) {
    >      >          >      >      >   131                              
instruction = pipe.readln();
    >      >          >      >      >   132                              
if(instruction.equals("check_done")) {
    >      >          >      >      >   133                                  if 
(test_thread.isAlive()) {
    >      >          >      >      >   134                                     
 exitCode = FAILED;
    >      >          >      >      >   135                                  }
    >      >          >      >      >    Space is missed after while and if.
    >      >          >      >      >    Some logErr("...") for failed case 
would be useful.
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      > Thanks,
    >      >          >      >      > Serguei
    >      >          >      >      >
    >      >          >      >      > On 2/26/19 6:06 PM, Daniil Titov wrote:
    >      >          >      >      > Hi Chris,
    >      >          >      >      >
    >      >          >      >      > Please review a new version of the webrev 
that slightly changes method receiveEvents(long,pattern)  in the following 
tests:
    >      >          >      >      >   
-vmTestbase/nsk/jdi/ClassUnloadRequest/addClassExclusionFilter/exclfilter001.java
    >      >          >      >      >   
-vmTestbase/nsk/jdi/ClassUnloadRequest/addClassFilter/filter001.java)
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      > The new changes ensure that 
receiveEvents(long,pattern)  method keeps receiving events in a while loop even 
if eventSet returned by  debugee.VM().eventQueue().remove(waitTime) is a null 
due to timeout.
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      > Bug: 
https://bugs.openjdk.java.net/browse/JDK-8207367
    >      >          >      >      > Webrev: 
http://cr.openjdk.java.net/~dtitov/8207367/webrev.02
    >      >          >      >      >
    >      >          >      >      > Thanks!
    >      >          >      >      > -Daniil
    >      >          >      >      >
    >      >          >      >      > On 2/26/19, 11:41 AM, "Chris Plummer" 
mailto:[email protected] wrote:
    >      >          >      >      >
    >      >          >      >      >      Ok. I think you mentioned below that 
default wait time will be 5
    >      >          >      >      >      seconds. That seems sufficient to 
avoid a timeout if there are some
    >      >          >      >      >      minor network issues and some 
packets are lost.
    >      >          >      >      >
    >      >          >      >      >      Changes look good.
    >      >          >      >      >
    >      >          >      >      >      thanks,
    >      >          >      >      >
    >      >          >      >      >      Chris
    >      >          >      >      >
    >      >          >      >      >      On 2/26/19 10:01 AM, Daniil Titov 
wrote:
    >      >          >      >      >      > Hi Chris,
    >      >          >      >      >      >
    >      >          >      >      >      > Yes , it is correct. For example 
in this particular test the timeout is expected (line 283 expects that 
breakpoint() returns returnCode3 that is set on line 460 when eventSet on line 
456 is null due to a timeout in eventQueue.remove()) and shortening it makes 
the whole test fit in jtreg time limits.
    >      >          >      >      >      >
    >      >          >      >      >      >    281                     log2("  
     checking up that the thread2 is not at breakpoint1 because of suspension");
    >      >          >      >      >      >     282                    
expresult = breakpoint();
    >      >          >      >      >      >     283                    if 
(expresult != returnCode3) {
    >      >          >      >      >      >     284                        
log3("ERROR: no timeout for waiting for BreakpointEvent when the thread2 is 
suspended");
    >      >          >      >      >      >     285                        
expresult = returnCode1;
    >      >          >      >      >      >     286                        
break label1;
    >      >          >      >      >      >     287                    } else
    >      >          >      >      >      >
    >      >          >      >      >      >
    >      >          >      >      >      >    445         private int 
breakpoint () {
    >      >          >      >      >      >     446
    >      >          >      >      >      >     447            int returnCode 
= returnCode0;
    >      >          >      >      >      >     448
    >      >          >      >      >      >     449            log2("       
waiting for BreakpointEvent");
    >      >          >      >      >      >     450
    >      >          >      >      >      >     451            labelBP:
    >      >          >      >      >      >     452                for (;;) {
    >      >          >      >      >      >     453
    >      >          >      >      >      >     454                    log2("  
     new:  eventSet = eventQueue.remove();");
    >      >          >      >      >      >     455                    try {
    >      >          >      >      >      >     456                        
eventSet = eventQueue.remove (Utils.adjustTimeout(waitTime*1000));
    >      >          >      >      >      >     457                        if 
(eventSet == null) {
    >      >          >      >      >      >     458                            
log2("::::::  timeout when waiting for a BreakpintEvent");
    >      >          >      >      >      >     459    //                      
  log3("ERROR:  timeout for waiting for a BreakpintEvent");
    >      >          >      >      >      >     460                            
returnCode = returnCode3;
    >      >          >      >      >      >     461                            
break labelBP;
    >      >          >      >      >      >     462                        }
    >      >          >      >      >      >
    >      >          >      >      >      >>     And I just noticed the space 
right after "remove". Can you remove it?
    >      >          >      >      >      > Sure. Will do.
    >      >          >      >      >      >
    >      >          >      >      >      > Thanks!
    >      >          >      >      >      > --Daniil
    >      >          >      >      >      >
    >      >          >      >      >      > On 2/25/19, 7:26 PM, "Chris 
Plummer" mailto:[email protected] wrote:
    >      >          >      >      >      >
    >      >          >      >      >      >      Hi Daniil,
    >      >          >      >      >      >
    >      >          >      >      >      >      For suspend001, are you 
saying the following is expected to timeout
    >      >          >      >      >      >      sometimes, so you need a 
shorter waittime to avoid making the whole test
    >      >          >      >      >      >      time out?
    >      >          >      >      >      >
    >      >          >      >      >      >        456                     
eventSet = eventQueue.remove
    >      >          >      >      >      >      
(Utils.adjustTimeout(waitTime*1000));
    >      >          >      >      >      >
    >      >          >      >      >      >      And I just noticed the space 
right after "remove". Can you remove it?
    >      >          >      >      >      >
    >      >          >      >      >      >      thanks,
    >      >          >      >      >      >
    >      >          >      >      >      >      Chris
    >      >          >      >      >      >
    >      >          >      >      >      >      On 2/25/19 6:57 PM, Daniil 
Titov wrote:
    >      >          >      >      >      >      > Hi Chris,
    >      >          >      >      >      >      >
    >      >          >      >      >      >      > The timeout issue mentioned 
in the bug is about jtreg aborting the tests since they are running longer than 
the maximum allowed time. That happens since these tests  use extreme long 
internal delays,  e.g. a sleep for 5 minutes  or a wait for 5 minutes for a 
case when no events ( and a notify()) are expected. Reducing these internal 
delays makes the test passing within the default jtreg timeout ( 2 minutes).
    >      >          >      >      >      >      >
    >      >          >      >      >      >      > Best regards,
    >      >          >      >      >      >      > Daniil
    >      >          >      >      >      >      >
    >      >          >      >      >      >      > On 2/25/19, 6:15 PM, 
"Chris Plummer" mailto:[email protected] wrote:
    >      >          >      >      >      >      >
    >      >          >      >      >      >      >      Ok. So how is the 
timeout issue mentioned in the bug addressed when
    >      >          >      >      >      >      >      there is now a shorter 
wait time?
    >      >          >      >      >      >      >
    >      >          >      >      >      >      >      Chris
    >      >          >      >      >      >      >
    >      >          >      >      >      >      >      On 2/25/19 5:04 PM, 
Daniil Titov wrote:
    >      >          >      >      >      >      >      > Hi Chris,
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      > Forgot to answer to 
your another question:
    >      >          >      >      >      >      >      >   >      > For these 
3 tests the event wait timeout was reduced and adjusted for test.timeout.factor:
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/Event/_itself_/event001.java
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/VirtualMachine/suspend/suspend001/TestDescription.java
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/ThreadReference/suspend/suspend001.java
    >      >          >      >      >      >      >      >      >      So 
overall is this a shorter or longer waittime now?
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      > Overall this is a 
shorter waitime now.  Instead of 300 seconds it is now 20 seconds for Mach5 
jobs (they are run with test.timeout.factor set to 4.0) and 5 seconds for 
regular jtreg runs.
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      > Best regards,
    >      >          >      >      >      >      >      > Daniil
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      > On 2/25/19, 4:38 
PM, "Chris Plummer" mailto:[email protected] wrote:
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >      Hi Daniil,
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >      Yes, my point 
was that the max time you wait for a single event is much
    >      >          >      >      >      >      >      >      smaller now. I 
can see a possibility that with a little bit of network
    >      >          >      >      >      >      >      >      instability  a 
packet gets lost and resend does not happen fast enough.
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >      thanks,
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >      Chris
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >      On 2/25/19 4:32 
PM, Daniil Titov wrote:
    >      >          >      >      >      >      >      >      > Hi Chris,
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      > The code 
still waits for the whole total wait time. There is a while loop at lines 
163-186 that keeps receiving new events (line 183) till elapsed time is less 
than the waittime (line 178) or a timeout happens (so eventSet is null).
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      > 159           
    begin = System.currentTimeMillis();
    >      >          >      >      >      >      >      >      >     160       
            eventSet = debugee.VM().eventQueue().remove(waitTime);
    >      >          >      >      >      >      >      >      >     161       
            delta = System.currentTimeMillis() - begin;
    >      >          >      >      >      >      >      >      >     162       
            totalWaitTime -= delta;
    >      >          >      >      >      >      >      >      >     163       
            while (eventSet != null) {
    >      >          >      >      >      >      >      >      >     164       
                EventIterator eventIterator = eventSet.eventIterator();
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >     178       
                if (totalWaitTime <= 0 || exit) {
    >      >          >      >      >      >      >      >      >     179       
                    break;
    >      >          >      >      >      >      >      >      >     180       
                }
    >      >          >      >      >      >      >      >      >     181       
                debugee.resume();
    >      >          >      >      >      >      >      >      >     182       
                begin = System.currentTimeMillis();
    >      >          >      >      >      >      >      >      >     183       
                eventSet = debugee.VM().eventQueue().remove(waitTime);
    >      >          >      >      >      >      >      >      >     184       
                delta = System.currentTimeMillis() - begin;
    >      >          >      >      >      >      >      >      >     185       
                totalWaitTime -= delta;
    >      >          >      >      >      >      >      >      >     186       
            }
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      > However, as I 
see now in case if a timeout happens on line 160  (eventSet is null) the loop 
is not executed at all.  I haven't observed it in test runs but I think it 
makes sense to adjust this test to take this potential case into account. I 
will send an updated version of the patch soon.
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      > Thanks!
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      > Best regards,
    >      >          >      >      >      >      >      >      > Daniil
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      > On 2/25/19, 
12:21 PM, "Chris Plummer" mailto:[email protected] wrote:
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      Hi 
Daniil,
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      On 
2/23/19 1:02 PM, Daniil Titov wrote:
    >      >          >      >      >      >      >      >      >      > Please 
review the change that fixes timeout issues for the following 10 tests when 
running with jtreg and default timeout factor (1.0).
    >      >          >      >      >      >      >      >      >      In 
Utils.java, I think wait() should be moved right after
    >      >          >      >      >      >      >      >      >      
waitForCondition() and maybe given a more descriptive name. It seems to
    >      >          >      >      >      >      >      >      >      
basically the same as waitForCondition(), except you added a "log"
    >      >          >      >      >      >      >      >      >      
parameter and slightly changed the behavior. Are these behavior
    >      >          >      >      >      >      >      >      >      
differences necessary? Could you share code with the existing
    >      >          >      >      >      >      >      >      >      
waitForCondition()?
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      > For 
the following 2 tests the event wait timeout was reduced and adjusted for 
test.timeout.factor.  Method receiveEvents(long,pattern) was fixed to ensure 
that it gracefully exits after the specified wait period elapsed:
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/ClassUnloadRequest/addClassExclusionFilter/exclfilter001.java
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/ClassUnloadRequest/addClassFilter/filter001.java
    >      >          >      >      >      >      >      >      >        183    
             eventSet = debugee.VM().eventQueue().remove(waitTime);
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      This 
code used to wait for the total remaining waittime. Now it waits a
    >      >          >      >      >      >      >      >      >      fixed 
amount based on:
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >        153    
     long waitTime = Utils.adjustTimeout(1000);
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      How did 
you come up with this wait amount, and is it long enough to deal
    >      >          >      >      >      >      >      >      >      with 
occasional hiccups?
    >      >          >      >      >      >      >      >      >      > For 
these 3 tests the event wait timeout was reduced and adjusted for 
test.timeout.factor:
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/Event/_itself_/event001.java
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/VirtualMachine/suspend/suspend001/TestDescription.java
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/ThreadReference/suspend/suspend001.java
    >      >          >      >      >      >      >      >      >      So 
overall is this a shorter or longer waittime now?
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      > For 
next 2 tests the event wait timeout and the thread sleep time were reduced and 
adjusted for test.timeout.factor. Additional synchronization between the 
debugger and the debuggee was added to ensure the debugee process continues as 
soon as the test finishes the timeout related checks and advances to the next 
steps:
    >      >          >      >      >      >      >      >      >      >    - 
vmTestbase/nsk/jdi/EventQueue/remove_l/remove_l004/TestDescription.java
    >      >          >      >      >      >      >      >      >      >    - 
vmTestbase/nsk/jdi/EventQueue/remove/remove004/TestDescription.java
    >      >          >      >      >      >      >      >      >      Ok.
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      > 
Instead of just sleeping for 5 minutes while waiting for the debuggee test 
thread to complete  the tests now check whether the debuggee thread is alive in 
the loop. The total waiting timeout was adjusted for test.timeout.factor:
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose004/TestDescription.java
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose003/TestDescription.java
    >      >          >      >      >      >      >      >      >      >    
-vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose002/TestDescription.java
    >      >          >      >      >      >      >      >      >      Ok.
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      thanks,
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      Chris
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      > 
Testing.
    >      >          >      >      >      >      >      >      >      > The 
following VM options were used  in Mach5 jobs to  verify these changes:
    >      >          >      >      >      >      >      >      >      > 1. No 
VM args
    >      >          >      >      >      >      >      >      >      > 2. 
-XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -XX:+TieredCompilation 
-XX:+UseJVMCICompiler -Djvmci.Compiler=grail
    >      >          >      >      >      >      >      >      >      > 3. 
-Xcomp
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      > Also 
tier1, tier2 and tier3 Mach5 jobs succeeded.
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      > To 
verify that tests succeed with test.timeout.factor set to 1.0 the following 
patch was used before running Mach5 jobs.
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      > --- 
a/make/RunTests.gmk Thu Feb 21 15:17:42 2019 -0800
    >      >          >      >      >      >      >      >      >      > +++ 
b/make/RunTests.gmk Thu Feb 21 15:42:15 2019 -0800
    >      >          >      >      >      >      >      >      >      > @@ 
-826,6 +826,7 @@
    >      >          >      >      >      >      >      >      >      >     
else
    >      >          >      >      >      >      >      >      >      >       
JTREG_TIMEOUT_FACTOR ?= 4
    >      >          >      >      >      >      >      >      >      >     
endif
    >      >          >      >      >      >      >      >      >      > +  
JTREG_TIMEOUT_FACTOR = 1
    >      >          >      >      >      >      >      >      >      >     
JTREG_VERBOSE ?= fail,error,summary
    >      >          >      >      >      >      >      >      >      >     
JTREG_RETAIN ?= fail,error
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      > Bug: 
https://bugs.openjdk.java.net/browse/JDK-8207367
    >      >          >      >      >      >      >      >      >      > 
Webrev: http://cr.openjdk.java.net/~dtitov/8207367/webrev.01
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      > Thanks!
    >      >          >      >      >      >      >      >      >      > 
--Daniil
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >      >
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >      >
    >      >          >      >      >      >      >
    >      >          >      >      >      >      >
    >      >          >      >      >      >      >
    >      >          >      >      >      >      >
    >      >          >      >      >      >      >
    >      >          >      >      >      >
    >      >          >      >      >      >
    >      >          >      >      >      >
    >      >          >      >      >      >
    >      >          >      >      >      >
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >      >
    >      >          >      >
    >      >          >      >
    >      >          >      >
    >      >          >      >
    >      >          >      >
    >      >          >
    >      >          >
    >      >          >
    >      >          >
    >      >          >
    >      >
    >      >
    >      >
    >      >
    >      >
    >      >
    >      
    >      
    >      
    >
    >
    
    


Reply via email to