RFR (L): 8241807: JDWP needs update for hidden classes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Apr 29 20:53:52 UTC 2020
Thank you, Alex!
Serguei
On 4/29/20 12:21, Alex Menkov wrote:
> LGTM
>
> --alex
>
> On 04/28/2020 18:31, serguei.spitsyn at 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 at 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 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