RFR JDK-8211292: [TEST] convert com/sun/jdi/DeferredStepTest.sh test

Alex Menkov alexey.menkov at oracle.com
Fri Oct 5 23:53:11 UTC 2018


ok, this is updated webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev.03/

--alex

On 10/05/2018 12:37, serguei.spitsyn at oracle.com wrote:
> In general, like the suggestion from Jc with the correction for lastLine 
> to be a local.
> But leave it up to Alex to decide what is better as changes would 
> require another round of testing.
> 
> Thanks,
> Serguei
> 
> On 10/5/18 12:10 PM, JC Beyler wrote:
>> You're right for the single threaded part; I misread that part and 
>> thought it would be multi-threaded as well. And fair enough for the 
>> keeping it then as a do..while(false); it just took me a while to 
>> figure out what was being done. You could put the data.lastLine in a 
>> local variable and update it at the start of the method (only using 
>> the local version for the rest of the method); then everything would 
>> be in there. But, I'll still say it is a more a question of style :)
>>
>> LGTM,
>> Jc
>>
>> On Fri, Oct 5, 2018 at 12:01 PM Alex Menkov <alexey.menkov at oracle.com 
>> <mailto:alexey.menkov at oracle.com>> wrote:
>>
>>     Hi Jc,
>>
>>
>>     On 10/05/2018 10:34, JC Beyler wrote:
>>     > Hi Alex,
>>     >
>>     > One question and a comment on this:
>>     > - I thought HashMap was not thread safe so I think you need to
>>     > synchronize the access to the map threadData
>>
>>     The map is accessed from a single thread (main test thread which
>>     sends
>>     jdb commands and handles jdb replies), so synchronization is not
>>     required.
>>
>>     >
>>     > - I think your test code could be simplified if you moved it into a
>>     > helper method (not tested but just for example):
>>
>>     I suppose you don't like do/break/while(false)?
>>     To me it's quite standard method to avoid multi-level if/then/else.
>>     In your suggestion I don't like that processNewData() method handles
>>     minLine/maxLine, but doesn't handle lastLine (i.e. it doesn't do all
>>     processing). But if "data.lastLine = lineNum" is moved into the
>>     method,
>>     we need something like do/break/while(false) in the method.
>>
>>     --alex
>>
>>     >
>>     > +    private void next() {
>>     > +        List<String> reply = jdb.command(JdbCommand.next());
>>     > +        /*
>>     > +         * Each "next" produces something like ("Breakpoint
>>     hit" line
>>     > only if the line has BP)
>>     > +         *   Step completed:
>>     > +         *     Breakpoint hit: "thread=jj2",
>>     > DeferredStepTestTarg$jj2.run(), line=74 bci=12
>>     > +         *     74                    ++count2;
>>     >   // @2 breakpoint
>>     > +         *     <empty line>
>>     > +         *     jj2[1]
>>     > +         */
>>     > +        // detect thread from the last line
>>     > +        String lastLine = reply.get(reply.size() - 1);
>>     > +        String threadName = parse(threadRegexp, lastLine);
>>     > +        String wholeReply =
>>     > reply.stream().collect(Collectors.joining(Utils.NEW_LINE));
>>     > +        int lineNum = Integer.parseInt(parse(lineRegexp,
>>     wholeReply));
>>     > +
>>     > +        System.out.println("got: thread=" + threadName + ",
>>     line=" +
>>     > lineNum);
>>     > +
>>     > +        ThreadData data = threadData.get(threadName);
>>     > +        if (data == null) {
>>     > +            data = new ThreadData();
>>     > +            threadData.put(threadName, data);
>>     > +        }
>>     > +        processNewData(data, threadName, lineNum);
>>     > +        data.lastLine = lineNum;
>>     > +    }
>>     > +
>>     > +  private void processNewData(ThreadData data, String
>>     threadName, int
>>     > lineNum) {
>>     > +        if (data.lastLine < 0) {
>>     > +            // the 1st stop in the thread
>>     > +            return;
>>     > +        }
>>     > +
>>     > +        if (lineNum == data.lastLine + 1) {
>>     > +            // expected.
>>     > +            return;
>>     > +        }
>>     > +
>>     > +        if (lineNum < data.lastLine) {
>>     > +            // looks like step to the beginning of the cycle
>>     > +            if (data.minLine > 0) {
>>     > +               // minLine and maxLine are not set - verify
>>     > +               Asserts.assertEquals(lineNum, data.minLine,
>>     threadName +
>>     > " - minLine");
>>     > +               Asserts.assertEquals(data.lastLine, data.maxLine,
>>     > threadName + " - maxLine");
>>     > +            } else {
>>     > +                // set minLine/maxLine
>>     > +                data.minLine = lineNum;
>>     > +                data.maxLine = data.lastLine;
>>     > +           }
>>     > +           return;
>>     > +        }
>>     > +
>>     > +        throw new RuntimeException(threadName + " (line " +
>>     lineNum +
>>     > ") - unexpected."
>>     > +            + " lastLine=" + data.lastLine + ", minLine=" +
>>     > data.minLine + ", maxLine=" + data.maxLine);
>>     > + }
>>     >
>>     > Thanks,
>>     > Jc
>>     >
>>     >
>>     >
>>     > On Thu, Oct 4, 2018 at 6:31 PM <serguei.spitsyn at oracle.com
>>     <mailto:serguei.spitsyn at oracle.com>
>>     > <mailto:serguei.spitsyn at oracle.com
>>     <mailto:serguei.spitsyn at oracle.com>>> wrote:
>>     >
>>     >     Hi Alex,
>>     >
>>     >     It looks good to me.
>>     >     Could you, please, also remove the line? :
>>     >
>>     >        156             //
>>     >
>>     >     No need in new webrev.
>>     >
>>     >     Thanks,
>>     >     Serguei
>>     >
>>     >
>>     >     On 10/4/18 4:11 PM, Alex Menkov wrote:
>>     >      > Hi Serguei,
>>     >      >
>>     >      > Updated webrev:
>>     >      >
>>     >
>>     http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev.02/
>>     <http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev.02/>
>>     >   
>>      <http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev.02/>
>>     >      >
>>     >      > Fixed all issues except
>>     >      >  140                 // the 1st stop in the thread
>>     >      >  141                 break;
>>     >      > In this case the comment is an explanation why we reach the
>>     >     block, not
>>     >      > an explanation for the "break" statement.
>>     >      >
>>     >      > --alex
>>     >      >
>>     >      > On 10/04/2018 13:56, serguei.spitsyn at oracle.com
>>     <mailto:serguei.spitsyn at oracle.com>
>>     >     <mailto:serguei.spitsyn at oracle.com
>>     <mailto:serguei.spitsyn at oracle.com>> wrote:
>>     >      >> Hi Alex,
>>     >      >>
>>     >      >> Several minor suggestions.
>>     >      >>
>>     >      >> 77 new Thread(aRP, "jj1").start();
>>     >      >> 78 new Thread(asRP, "jj2").start(); What mean aRP and
>>     asRP? In
>>     >     fact,
>>     >      >> it is confusing. Can they be renamed to something like
>>     obj1 and
>>     >     obj2.
>>     >      >>
>>     >      >> 79 // new Thread(aRP, "jj3").start();
>>     >      >> 80 // new Thread(asRP, "jj4").start();
>>     >      >>
>>     >      >>   These lines can be removed.
>>     >      >>
>>     >      >> 94 // line of the last stop
>>     >      >> 95 int lastLine = -1;
>>     >      >> 96 // min line (-1 means "not known yet")
>>     >      >> 97 int minLine = -1;
>>     >      >> 98 // max line (-1 means "not known yet")
>>     >      >> 99 int maxLine = -1; ... 140 // the 1st stop in the thread
>>     >      >> 141 break;
>>     >      >>
>>     >      >>    I'd suggest the refactor above as below:
>>     >      >>
>>     >      >> int lastLine = -1;  // line of the last stop
>>     >      >> int minLine = -1;  // min line (-1 means "not known yet")
>>     >      >> int maxLine = -1;// max line (-1 means "not known yet")
>>     >      >>   ...
>>     >      >> break;  // the 1st stop in the thread
>>     >      >>
>>     >      >> 116 private void next() {
>>     >      >> 117 List<String> reply = jdb.command(JdbCommand.next());
>>     >      >> 118 /* each "next" produces something like ("Breakpoint
>>     hit" line
>>     >      >> only if the line has BP)
>>     >      >> 119 Step completed:
>>     >      >> 120 Breakpoint hit: "thread=jj2",
>>     DeferredStepTestTarg$jj2.run(),
>>     >      >> line=74 bci=12
>>     >      >> 121 74 ++count2; // @2 breakpoint
>>     >      >> 122 <empty line>
>>     >      >> 123 jj2[1]
>>     >      >> 124 */ It would better to have it in this style: 118 /*
>>     * Each
>>     >     "next"
>>     >      >> produces something like ("Breakpoint hit" line only if
>>     the line
>>     >     has BP).
>>     >      >> 119 * Step completed:
>>     >      >> 120 * Breakpoint hit: "thread=jj2",
>>     DeferredStepTestTarg$jj2.run(),
>>     >      >> line=74 bci=12
>>     >      >> 121 * 74 ++count2; // @2 breakpoint
>>     >      >> 122 * <empty line>
>>     >      >> 123 * jj2[1]
>>     >      >> 124 */
>>     >      >>
>>     >      >>
>>     >      >> Otherwise, it looks Okay to me.
>>     >      >>
>>     >      >>
>>     >      >> Thanks,
>>     >      >> Serguei
>>     >      >>
>>     >      >> On 10/3/18 5:49 PM, Alex Menkov wrote:
>>     >      >>> Hi all,
>>     >      >>>
>>     >      >>> please review a fix for
>>     >      >>> https://bugs.openjdk.java.net/browse/JDK-8211292
>>     >      >>> webrev:
>>     >      >>>
>>     >
>>     http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev/
>>     <http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev/>
>>     >   
>>      <http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev/>
>>     >      >>>
>>     >      >>> The fix converts manual shell test to automatic java
>>     (as Java
>>     >     allows
>>     >      >>> to parse jdb replies much easier).
>>     >      >>> This is the last sub-task for the "shell to java
>>     conversion" task,
>>     >      >>> so the fix also removes shared shell scripts.
>>     >      >>>
>>     >      >>> --alex
>>     >      >>
>>     >
>>     >
>>     >
>>     > --
>>     >
>>     > Thanks,
>>     > Jc
>>
>>
>>
>> -- 
>>
>> Thanks,
>> Jc
> 


More information about the serviceability-dev mailing list