Kevin,
* CommandProcessor.java: "quit" should be volatile. * HSDB.java: argError logic is not clean to me. Is it possible to just do return after doUsage or use System.exit()? * HotSpotAgent.java: 1. You can probably check whether "debugger" variable is already set or not and get rid of "doAttach" and thus simplify code a bit. 2. setupDebuggerAlternate doesn't depend to value of os and cpu, so it might have sense to move this call to upper level. 672: Comment is redundant. * VM.java: After your changes some code in static initializer e.g. disableDerivedPrinterTableCheck is executed ever in case of error. I'm not sure we should try to recover here - if we can't load properties, something very bad is happening. LinuxAddress.java, LinuxOopHandle.java : What is the reason to make it public? * ClassDump.java: Does these changes related to this CR? On 2013-06-24 15:24, Kevin Walls wrote: > Thanks Staffan, > > Yes, the GUI does still close and the standalone HSDB ends, without > forcibly terminating the process if it's initiated by some other app. > > Can I get anybody else interested in reviewing, commenting, etc...? > > Thanks > Kevin > > > On 24/06/13 12:18, Staffan Larsen wrote: >> On 24 jun 2013, at 11:20, Staffan Larsen<staffan.lar...@oracle.com> >> wrote: >> >>> On 19 jun 2013, at 14:40, Kevin Walls<kevin.wa...@oracle.com> wrote: >>> >>>> Hi Staffan -- >>>> >>>> And apologies from me for the slow turnaround. 8-) >>>> >>>> Thanks for the suggestions, implementing them all. >>> Thanks. >>> >>>> http://cr.openjdk.java.net/~kevinw/8010278/webrev.02/ >>>> >>>> Also adding the same kind of change to the HSDB tool, a few changes >>>> there to get the gui closing without using System.exit. >>> So how do I now exit the HSDB tool? Closing the window won't exit. >>> File->Exit won't exit. Or did I miss something? >> I did miss that the workerThread is also terminated in closeUI() and >> that causes the app to exit. >> >> This looks good. Reviewed. >> >> /Staffan >> >>> /Staffan >>> >>>> Additional feedback gratefully received... >>>> >>>> Thanks >>>> Kevin >>>> >>>> >>>> On 05/06/13 12:00, Staffan Larsen wrote: >>>>> Some comments (sorry it took so long): >>>>> >>>>> CLHSDB.java >>>>> - Can you move the jvmDebugger field to where the other fields are >>>>> in the class? Make it private, too. >>>>> - Remove start() and make run() public? >>>>> - Perhaps improve on the comment in run() saying that either >>>>> jvmDebugger OR pidText OR {execPath, coreFileName} should be set. >>>>> >>>>> HotspotAgent.java >>>>> - Should sa.altHotSpotAgent be called sa.altDebugger ? >>>>> >>>>> Tool.java >>>>> - Make jvmDebugger private. >>>>> >>>>> >>>>> Regards, >>>>> /Staffan >>>>> >>>>> >>>>> >>>>> >>>>> On 23 maj 2013, at 15:23, Kevin Walls<kevin.wa...@oracle.com> wrote: >>>>> >>>>>> Forgot to mention: >>>>>> >>>>>> I moved the ClassDump Tool around a little so it was usable via a >>>>>> route other than calling main(), and added the constructor that >>>>>> takes a String for the pkgList, which saves using the system >>>>>> property to communicate what classes you want to dump >>>>>> (sun.jvm.hotspot.tools.jcore.PackageNameFilter has that >>>>>> constructor already). >>>>>> >>>>>> Actually, considering the package filter name is always going to >>>>>> be sun.jvm.hotspot.tools.jcore.PackageNameFilter, let's have that >>>>>> as a default value when we call getProperty. Similarly the >>>>>> getProperty for outputDir, and at that point I stop tweaking. >>>>>> >>>>>> A little indenting was off also and I added a comment, so I redid >>>>>> the webrev: >>>>>> >>>>>> http://cr.openjdk.java.net/~kevinw/8010278/webrev.01/ >>>>>> >>>>>> Thanks >>>>>> Kevin >>>>>> >>>>>> >>>>>> >>>>>> On 23/05/13 11:00, Kevin Walls wrote: >>>>>>> Hi, >>>>>>> >>>>>>> As the Serviceability Agent has been _the_ new and interesting >>>>>>> way to find things post-mortem in the JVM [1], I'd like to >>>>>>> propose an update which continues that tradition. >>>>>>> >>>>>>> 8010278 SA: provide mechanism for using an alternative SA >>>>>>> debugger back-end. >>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8010278 >>>>>>> http://cr.openjdk.java.net/~kevinw/8010278/webrev.00/ >>>>>>> >>>>>>> This is about making the SA more flexible, so we aren't tied to >>>>>>> the given native libraries to open cores/memory dumps. Given >>>>>>> this change, a 3rd party debugger or tool can interact freely >>>>>>> with the SA tools (StackTrace, ObjectHistogram, etc...) and >>>>>>> provide its own backend implementation to actually open a >>>>>>> core/memory dump. >>>>>>> >>>>>>> Primarily for platform-independent core file debugging. If you >>>>>>> ever had to open a "foreign" core, find the right hardware, >>>>>>> etc... this is relevant. I'm thinking of >>>>>>> https://java.net/projects/kjdb which can serve as a proof of >>>>>>> concept. >>>>>>> >>>>>>> The changes are: >>>>>>> >>>>>>> The main redirection is in HotSpotAgent.java, where we respect a >>>>>>> property (i.e. -Dsa.altHotSpotAgent=...) to name an alternate >>>>>>> debugger. >>>>>>> >>>>>>> Remove calls to System.exit. >>>>>>> >>>>>>> Tool classes (and CLHSDB) should have a constructor that takes a >>>>>>> JVMDebugger, to remove the assumption that a Tool's JVM will only >>>>>>> ever contain one debugee. It doesn't address that VM is a >>>>>>> singleton and if a tool opens multiple sessions then they would >>>>>>> need to be from the same JVM version. >>>>>>> >>>>>>> >>>>>>> Thanks >>>>>>> Kevin >>>>>>> >>>>>>> [1] If you weren't in a circa 1.4.2 demo of the SA when all you >>>>>>> had previously was a few fragile dbx macros, that got you a few >>>>>>> very specific details, the night vs. day comparison of no SA vs. >>>>>>> SA could be missed. 8-) >>>>>>> >>>>>>> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
