RFR (L): 8241807: JDWP needs update for hidden classes
Alex Menkov
alexey.menkov at oracle.com
Wed Apr 29 19:21:00 UTC 2020
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