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

Alex Menkov alexey.menkov at oracle.com
Fri Oct 5 19:01:15 UTC 2018


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>> 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/>
>      >
>      > 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> 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/>
>      >>>
>      >>> 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


More information about the serviceability-dev mailing list