Thanks Kevin, You answered all my questions ;)
-Dmitry On 2013-06-24 18:53, Kevin Walls wrote: > Thanks Dmitry -- > > I'll come back with an update, but just a couple of responses... > > > > On 24/06/13 15:01, Dmitry Samersoff wrote: >> On 2013-06-24 17:48, Kevin Walls wrote: >>> Thanks Dmitry -- >>> >>> >>> CommandProcessor.java: >>> Yes, should make quit volatile. >>> >>> >>> HSDB.java: >>> The idea is to avoid System.exit as it is really unfriendly when another >>> app invokes the tool. The existing usage pattern is: >>> >>> new HSDB(args).run(); >>> >>> ..so avoiding System.exit() means making run() guard against being >>> called when the args were bad, i.e. the usage error was issued. I could >>> change main() here to check something and not call run, but the general >>> pattern of the tools is to instantiate them and call run(), so I thought >>> it best that run should have this check? >> I don't fill myself comfortable with create a thread and just exit >> because of a previous error. So IMHO, it's better to don't call run at >> all. >> > > > I don't think we're creating a new thread for HSDB? > (although Tool implements Runnable, HSDB doesn't extend Tool like other > classes do, if that's where this comes from?) > > I think run() needs to protect itself from creating a GUI when the app > should not be up, due to an argument error. The flag could have been > called "initialized" but that is misleading: there's a lot more > "initialisation" yet to come when you get to run()! > > Let me know if you have more thoughts on what makes you uncomfortable > here. 8-) > > > >>> HotSpotAgent.java: >>> >>> 1 >>> Yes maybe we don't need the doAttach flag. I'll try that out. >> OK. Thank you! >> >>> 2 >>> Do you mean to not have the call to setupDebuggerAlternate() alongside >>> the alternative calls to setupDebuggerSolaris/Linux/Win32/etc...? >>> >>> Maybe it doesn't have the same dependencies as those methods, but it >>> does do the kinds of things: >>> call setupJVMLibNames, and also set the MachineDescription. I think it >>> is still a parallel method to those for the other platforms, so this >>> seems like the appropriate point to call it? If we move the call to >>> somewhere earlier, we would still have to check the "sa.altDebugger" >>> property in setupDebugger(), to make sure we DON'T call one of the other >>> setupDebuggerXX methods. >>> >>> (If I didn't properly understand what you meant, let me know!) >> Now if we try to run SA on non-supported platform CPU combination it >> fails ever if we provide our own backend for this platform. >> >> I'm not sure - is it OK or not. >> > > Yes, I see what you mean, the UnsupportedPlatformException - we are > still tied to the running JVM's PlatformInfo implementation. I should > be able to reorder this a little to solve that... > > Thanks > Kevin > > >>> 3 >>> Oh you found my "XXX" comment, yes... 8-) >>> That needs tidying up. >>> >>> >>> VM.java: >>> On the properties, we are looking up a build number so we can issue a >>> warning about a mismatch. >>> >>> I had also noticed we corrupt "derivedPointer" to "derivedPrinter" in a >>> few places, which I'll correct while doing this. 8-) >>> >>> Yes disableDerivedPrinterTableCheck (which we should call >>> disableDerivedPointerTableCheck in future) is set even if you can't read >>> the "sa.properties" file, but it's set from System.getProperties, i.e. >>> not related, just happens to be another part of the static {} block. >>> That seems OK? >> OK. >> >>> LinuxAddress.java, LinuxOopHandle.java : >>> Added public so they are accessible to other code, e.g. so an external >>> unrelated tool can create a LinuxAddress. >> It's better to add a comment that these classes is intended to be used >> by external tools. >> >>> ClassDump.java: >>> The change here is about letting it get initialized somewhere other than >>> main(). i.e if the Tool is invoked without main, and originally main is >>> where it initializes its class filter and output directory. (It doesn't >>> use the args from main for these) So it's moved... >> Could you add a comment explaining it? >> >>> Let me rebuild and test with these changes and come back with an updated >>> webrev. >> Thanks! >> -Dmitry >> >>> Thanks >>> Kevin >>> >>> >>> >>> On 24/06/13 13:28, Dmitry Samersoff wrote: >>>> 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.