RFR (L): 8241807: JDWP needs update for hidden classes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Apr 29 01:03:33 UTC 2020
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 at 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/test/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/test/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/test/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 at 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
>>
More information about the serviceability-dev
mailing list