[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (L): 8241807: JDWP needs update for hidden classes
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2020-04-29 20:53:52
Message-ID: af665b5a-338a-2bfd-084d-519517da35e4 () oracle ! com
[Download RAW message or body]
Thank you, Alex!
Serguei
On 4/29/20 12:21, Alex Menkov wrote:
> LGTM
>
> --alex
>
> On 04/28/2020 18:31, serguei.spitsyn@oracle.com wrote:
> > Hi Alex,
> >
> > The updated webrev version is:
> > http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.6/
> >
> > I'll push it if you have no objections.
> >
> > Thanks,
> > Serguei
> >
> >
> >
> > On 4/28/20 18:03, serguei.spitsyn@oracle.com wrote:
> > > Hi Alex,
> > >
> > > Thank you a lot for reviewing this.
> > >
> > >
> > > On 4/28/20 17:35, Alex Menkov wrote:
> > > > Hi Serguei,
> > > >
> > > > Looks good in general.
> > > > couple minor notes:
> > > >
> > > > DebuggerBase.java
> > > >
> > > > - vm, debuggee, pipe and erManager fields and getDebuggee(), vm()
> > > > methods don't need to be static;
> > >
> > > Thank you for the suggestion.
> > >
> > > The erManager and pipe are not static.
> > > It feels like you need to refresh the webrev pages as I've updated
> > > the webrev in place a couple of times while waited for review.
> > >
> > > The vm() method is used in the EventHandler class which does not
> > > have a reference to the DebuggerBase object.
> > > It is why the vm() needs to be static.
> > > The same is true for 4 other methods: getReferenceType(),
> > > findField(), findMethod() and invokeStaticMethod().
> > > I was also thinking on how to get rid of this staic-ness and decided
> > > to leave it alone.
> > >
> > > The getDebuggee() method is not used yet.
> > > I've removed it. It is not a problem to add it back in case it is
> > > needed.
> > >
> > > >
> > > > - replace all "imultiple" with "multiple".
> > >
> > > Nice catch - fixed.
> > >
> > > Thanks,
> > > Serguei
> > >
> > > >
> > > > --alex
> > > >
> > > >
> > > > On 04/27/2020 18:34, serguei.spitsyn@oracle.com wrote:
> > > > > Hi Leonid,
> > > > >
> > > > > Updated webrev version with the suggested refactoring is:
> > > > > http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.5/
> > > > > <http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.5/>
> > > > >
> > > > >
> > > > > <http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.5/>
> > > > >
> > > > >
> > > > > Please, see my comments below.
> > > > >
> > > > > On 4/24/20 13:20, Leonid Mesnik wrote:
> > > > > >
> > > > > > Hi
> > > > > >
> > > > > > I have several comments about coding and desing. These tests
> > > > > > might be used as examples for new JDI tests based on this
> > > > > > framework so I think it would be better to polish them.
> > > > > >
> > > > >
> > > > > Yes, as we agreed it'd be nice to create good examples, a base for
> > > > > a future test development in the JDI area.
> > > > > I also wanted to polish these tests as much as possible, so thanks
> > > > > for helping with it.
> > > > > These are good suggestions from you below.
> > > > > One problem is that these tests will differ in style from the rest
> > > > > of the NSK JDI tests but it has to be okay.
> > > > >
> > > > >
> > > > > > General comment:
> > > > > >
> > > > > > 1. You often check results (not null, exactly one result).
> > > > > >
> > > > > > It makes sense to use jdk.test.lib.Asserts for such verification.
> > > > > > Just to reduce number of code and fail with standard exceptions.
> > > > > >
> > > > >
> > > > > Good suggestion - fixed now.
> > > > >
> > > > > > 2. Wildcard imports like 'import com.sun.jdi.*;' are not
> > > > > > recommended.
> > > > > >
> > > > >
> > > > > Good suggestion - fixed.
> > > > > The list of imports became big but I see no problem with that.
> > > > >
> > > > > > 3. I recommend to catch Throwable instead of Exception especially
> > > > > > in non-main threads. Just to ensure that nothing is missed.
> > > > > >
> > > > >
> > > > > God suggestion - fixed, at least, in places where it makes sense
> > > > > to do.
> > > > >
> > > > > > 4. nsk.share.Failure is subclass of RuntimeException so you don't
> > > > > > need to add it to throws. Now sometimes it is added, sometimes
> > > > > > not. It looks inconsistent.
> > > > > >
> > > > >
> > > > > Good suggestion - fixed.
> > > > > In fact, I did not like the use of Failure as it creates this
> > > > > inconsistency, so I tried to get rid of it.
> > > > >
> > > > > > 5. There are lot of code duplication in debugger and debugge
> > > > > > classes. Does it make a sense to merge it into base classes?
> > > > > >
> > > > >
> > > > > I was thinking about this too but was reluctant to do so, as there
> > > > > are just two tests.
> > > > > But it has to be beneficial to move shared code to the JDI testing
> > > > > framework.
> > > > > I've just made some initial preparation by separating common parts
> > > > > DebuggeeBase and DebuggerBase.
> > > > > Also, it seems to be natural to move the HiddenClass and
> > > > > EventHandler classes to their own files.
> > > > > Then I've decided to combine both tests into one.
> > > > > All refactoring still makes sense to have.
> > > > >
> > > > > > http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/te \
> > > > > > st/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent/classPrepEvent001.java.html \
> > > > > >
> > > > > >
> > > > > > 6. The getField can't return null, it verifies result. So check
> > > > > > in line 191-193 is not needed.
> > > > > >
> > > > > > 190 Field field = getField(hcRefType, "hcField");
> > > > > > 191 if (field == null) {
> > > > > > 192 throw new Failure("TEST FAILED: cannot enable
> > > > > > ModificationWhatchpointRequest: hcField not found");
> > > > > > 193 }
> > > > >
> > > > > Good catch - fixed.
> > > > >
> > > > > >
> > > > > > 7. I would recommend to move all initialization from run in
> > > > > > constructor and make most of variable 'final' it helps to ensure
> > > > > > that they are initialized.
> > > > > > 297 public int run(final String args[], final PrintStream
> > > > > > out) {
> > > > > > 298 ArgumentHandler argHandler = new
> > > > > > ArgumentHandler(args);
> > > > > > 299
> > > > > > 300 log = new Log(out, argHandler);
> > > > > > 301 eventTimeout = argHandler.getWaitTime() * 60 *
> > > > > > 1000; // milliseconds
> > > > > > 302 launchDebuggee(argHandler);
> > > > >
> > > > > Right comment in general, but I did not go with a constructor and
> > > > > final fields.
> > > > > Instead, I've just reorganized this a little bit.
> > > > > There is a little base to refactor to the constructor because the
> > > > > field "log" needs to be static and timeout can be local.
> > > > >
> > > > >
> > > > > > 8. Pair looks inconsistent. start saves handler as class field
> > > > > > while join use as parameter.
> > > > > > 305 startEventHandler();
> > > > > > 330 joinEventHandler(eventHandler);
> > > > > >
> > > > > > I think that something like
> > > > > > EventHandler eventHandler = startEventHandler();
> > > > > > joinEventHandler(eventHandler);
> > > > > > looks better and restrict usage of eventHandler. You also migt
> > > > > > move methods startEventHandler into EventHandler itself.
> > > > >
> > > > > Good suggestion - fixed. Moved both methods to the EventHandler
> > > > > class.
> > > > >
> > > > > >
> > > > > > 9. I suggest to make setHCRefType private and rename getHCRefType
> > > > > > to something like receiveHCRefType. To make more clear that it is
> > > > > > not a simple getter, but method which wait for event.
> > > > > > 361 // Save hidden class ReferenceType when its
> > > > > > ClassPrepare event is received.
> > > > > > 362 synchronized void setHCRefType(ReferenceType type) {
> > > > > > 363 hcRefType = type;
> > > > > > 364 notifyAll();
> > > > > > 365 }
> > > > > > 366
> > > > > > 367 // This method is used by the debugger main thread
> > > > > > to wait and get
> > > > > > 368 // the hidden class reference type from its
> > > > > > ClassPrepare event.
> > > > > > 369 // The readyCmdSync with the debuggeee is not
> > > > > > enough because a
> > > > > > 370 // ClassPrepare event sent over JDWP protocol has
> > > > > > some delay.
> > > > > > 371 // A wait/notify sync is to ensure the debugger
> > > > > > gets non-null value.
> > > > > > 372 synchronized ReferenceType getHCRefType() {
> > > > > > 373 try {
> > > > > > 374 while (hcRefType == null) {
> > > > > > 375 wait();
> > > > > > 376 }
> > > > > > 377 } catch (InterruptedException ie) {
> > > > > > 378 throw new Failure("Unexpected
> > > > > > InterruptedException in waiting for HC prepare: " + ie);
> > > > > > 379 }
> > > > > > 380 return hcRefType;
> > > > > > 381 }
> > > > >
> > > > > Good suggestion - fixed. I've replaced getHCRefType with
> > > > > waitGetHCRefType.
> > > > > Also, I've removed all uses of Failure and InterruptedException
> > > > > catches and specified a couple of methods to throw
> > > > > InterruptedException.
> > > > >
> > > > > >
> > > > > > 10. We don't run tests with -ea/esa so assertions usually
> > > > > > disabled. Use Assertions from test lib instead. The comment 1) is
> > > > > > recommendation only but 'assert' is just ignored.
> > > > > > 436 assert name.indexOf("HiddenClass") > 0 &&
> > > > > > name.indexOf("/0x") > 0;
> > > > >
> > > > > Thanks - fixed.
> > > > >
> > > > > >
> > > > > > http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/te \
> > > > > > st/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent/classPrepEvent001a.java.html \
> > > > > >
> > > > > >
> > > > > >
> > > > > > 11. Check if jdk.test.lib.classloader.ClassLoadUtils.
> > > > > > getClassPath(String className) works for you instead of
> > > > > > 70 private static String getClassPath(Class<?> klass) {
> > > > > >
> > > > > > 12. You could use try(log = new PrintStream("Debuggee.log")) {}
> > > > > > here:
> > > > > > 82 log = new PrintStream("Debuggee.log");
> > > > > > ..
> > > > > > 86 try {
> > > > > > ..
> > > > > > 90 }
> > > > > > 91 log.close();
> > > > >
> > > > > Good idea, but unfortunately it does not work as the field needs
> > > > > to be static.
> > > > >
> > > > >
> > > > > > 13. It is better to throw RuntimeException and don't add throws
> > > > > > Exception in all methods.
> > > > > > 95 private void syncWithDebugger(IOPipe pipe) throws Exception {
> > > > >
> > > > > Good suggestion - fixed.
> > > > >
> > > > >
> > > > > > http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/te \
> > > > > > st/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassUnloadEvent/classUnloadEvent001a.java.html \
> > > > > >
> > > > > >
> > > > > > You could use single if for this.
> > > > > > 122 if (command == null) {
> > > > > > 123 throw new Exception("Failed: pipe.readln()
> > > > > > returned null instead of COMMAND_QUIT");
> > > > > > 124 } else if
> > > > > > (!command.equals(classUnloadEvent001.COMMAND_QUIT)) {
> > > > > > 125 throw new Exception("Failed COMMAND_QUIT sync
> > > > > > with debugger, got unknown command: " + command);
> > > > > > 126 }
> > > > >
> > > > > I wanted a separate error message for (command == null) as it was
> > > > > related to OOME on the debuggee side.
> > > > > But it is not important anymore after the WT.fullGC() is used.
> > > > > Fixed.
> > > > >
> > > > > > 14. Setting to null is not required to collect objects here.
> > > > > > 151 hcObj = null;
> > > > > > 152 hc = null;
> > > > >
> > > > > It is better to keep these to be sure the hidden class can be
> > > > > unloaded.
> > > > > I had to create one mode hidden class to get a ClassUnload event.
> > > > > There is a comment in the test debuggee explaining it.
> > > > >
> > > > >
> > > > > > Probably I didn't catch all issues, but let discuss this list first.
> > > > >
> > > > > Okay.
> > > > >
> > > > > Thanks!
> > > > > Serguei
> > > > >
> > > > > > Leonid
> > > > > >
> > > > > > On 4/23/20 9:01 PM, serguei.spitsyn@oracle.com wrote:
> > > > > > > Please, review a fix for the sub-task:
> > > > > > > https://bugs.openjdk.java.net/browse/JDK-8241807
> > > > > > >
> > > > > > > Webrev:
> > > > > > > http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/ \
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Summary;
> > > > > > > This sub-task is to cover any hidden class related remaining
> > > > > > > issues in the JDI and JDWP implementation.
> > > > > > > In fact, no more issues were discovered with this extra test
> > > > > > > coverage.
> > > > > > > So, this update includes only two new nsk jdi tests:
> > > > > > > > >
> > > > > > > test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent
> > > > > > > > >
> > > > > > > test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassUnloadEvent
> > > > > > >
> > > > > > > These tests are to provide a test coverage which was
> > > > > > > impossible to provide with the jdb testing framework.
> > > > > > > It includes ClassPrepare, Breakpoint and
> > > > > > > ModificationWatchpoint events with class filters.
> > > > > > >
> > > > > > > Testing:
> > > > > > > The mach5 job is in progress:
> > > > > > > https://mach5.us.oracle.com/mdash/jobs/sspitsyn-HiddenClass-jdi-event-tests-20200424-0350-10464032 \
> > > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Serguei
> > > > >
> > >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic