RFR (L): 8241807: JDWP needs update for hidden classes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Apr 29 01:31:17 UTC 2020
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