PING: RFR: 8209790: SA tools not providing option to connect to debug server
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sun Jun 16 21:44:38 UTC 2019
Hi Yasumasa,
On 6/16/19 07:22, Yasumasa Suenaga wrote:
> Hi Serguei,
>
> >>> One minor suggestion is to use the final field NO_REMOTE instead
> >>> of null for initialization of the local variable "remote".
>
> I fixed it on new webrev. Could you check again?
> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.02/
It looks good.
Thanks you for the update!
>
> >> IMHO refactoring should be worked on another issue.
> >
> > Agreed.
>
> I filed it to JBS:
> https://bugs.openjdk.java.net/browse/JDK-8226204
Thank you for filing the enhancement!
Thanks.
Serguei
> Thanks,
>
> Yasumasa
>
>
> On 2019/06/15 15:10, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>>
>> On 6/14/19 21:11, Yasumasa Suenaga wrote:
>>> Hi Serguei,
>>>
>>> Thank you for your comment!
>>>
>>> On 2019/06/15 8:00, serguei.spitsyn at oracle.com wrote:
>>>> Hi Yasumasa,
>>>>
>>>> I've added myself as a reviewer, so you can finalize it now.
>>>
>>> I moved CSR to Finalized, and added a comment for your question.
>>
>> Okay, thanks!
>>
>>
>>>> The fix looks pretty good to me.
>>>>
>>>> One minor suggestion is to use the final field NO_REMOTE instead
>>>> of null for initialization of the local variable "remote".
>>>
>>> I will fix that.
>>>
>>>
>>>> Also just an observation that there is some room for option
>>>> processing refactoring.
>>>
>>> Your suggestion handles all options in one parser method.
>>> I concern it might be complex for option validation.
>>> (e.g. `jmap -heap` is allowed, but `jstack -heap` is not allowed.)
>>
>> This concern is not valid as the list allowed options allowed for each
>> jhsdb sub-command is controlled with the longOpts argument.
>>
>> jmap has:
>> String[] longOpts = {"exe=", "core=", "pid=", "remote=",
>> "heap", "binaryheap", "dumpfile=", "histo",
>> "clstats", "finalizerinfo"};
>>
>> but jstack has:
>> String[] longOpts = {"exe=", "core=", "pid=", "remote=",
>> "mixed", "locks"};
>>
>>> IMHO refactoring should be worked on another issue.
>>
>> Agreed.
>>
>>
>>> If you are OK in above, I will upload new webrev.
>>
>> Yes, I'm Okay with it.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>>> All the jhsdb sub-commands do execute similar loops like this:
>>>> while((s = sg.next(null, longOpts)) != null) {
>>>> . . .
>>>> }
>>>>
>>>> It can be moved into a separate method instead.
>>>> The longOpts can passed in arguments.
>>>>
>>>> It can be something like this:
>>>>
>>>> private ArrayList<String> processOptions(final String[] oldArgs,
>>>> final String[] longOpts,
>>>> boolean allowEmpty) {
>>>> SAGetopt sg = new SAGetopt(oldArgs);
>>>> ArrayList<String> newArgs = new ArrayList();
>>>>
>>>> String pid = null;
>>>> String exe = null;
>>>> String core = null;
>>>> String s = null;
>>>> String dumpfile = null;
>>>> String remote = NO_REMOTE;
>>>> boolean requestHeapdump = false;
>>>>
>>>> while((s = sg.next(null, longOpts)) != null) {
>>>> if (s.equals("exe")) {
>>>> exe = sg.getOptarg();
>>>> continue;
>>>> }
>>>> if (s.equals("core")) {
>>>> core = sg.getOptarg();
>>>> continue;
>>>> }
>>>> if (s.equals("pid")) {
>>>> pid = sg.getOptarg();
>>>> continue;
>>>> }
>>>> if (s.equals("remote")) {
>>>> remote = sg.getOptarg();
>>>> continue;
>>>> }
>>>> if (s.equals("mixed")) {
>>>> newArgs.add("-m");
>>>> continue;
>>>> }
>>>> if (s.equals("locks")) {
>>>> newArgs.add("-l");
>>>> continue;
>>>> }
>>>> if (s.equals("heap")) {
>>>> newArgs.add("-heap");
>>>> continue;
>>>> }
>>>> if (s.equals("binaryheap")) {
>>>> requestHeapdump = true;
>>>> continue;
>>>> }
>>>> if (s.equals("dumpfile")) {
>>>> dumpfile = sg.getOptarg();
>>>> continue;
>>>> }
>>>> if (s.equals("histo")) {
>>>> newArgs.add("-histo");
>>>> continue;
>>>> }
>>>> if (s.equals("clstats")) {
>>>> newArgs.add("-clstats");
>>>> continue;
>>>> }
>>>> if (s.equals("finalizerinfo")) {
>>>> newArgs.add("-finalizerinfo");
>>>> continue;
>>>> }
>>>> if (s.equals("flags")) {
>>>> newArgs.add("-flags");
>>>> continue;
>>>> }
>>>> if (s.equals("sysprops")) {
>>>> newArgs.add("-sysprops");
>>>> continue;
>>>> }
>>>> if (s.equals("serverid")) {
>>>> String serverid = sg.getOptarg();
>>>> if (serverid != null) {
>>>> newArgs.add(serverid);
>>>> }
>>>> continue;
>>>> }
>>>> }
>>>>
>>>> if (!requestHeapdump && (dumpfile != null)) {
>>>> throw new IllegalArgumentException("Unexpected
>>>> argument dumpfile");
>>>> }
>>>> if (requestHeapdump) {
>>>> if (dumpfile == null) {
>>>> newArgs.add("-heap:format=b");
>>>> } else {
>>>> newArgs.add("-heap:format=b,file=" + dumpfile);
>>>> }
>>>> }
>>>> buildAttachArgs(newArgs, pid, exe, core, remote, allowEmpty);
>>>>
>>>> return newArgs;
>>>> }
>>>>
>>>> private static void runCLHSDB(String[] oldArgs) {
>>>> String[] longOpts = {"exe=", "core=", "pid="};
>>>> ArrayList<String> newArgs = processOptions(oldArgs,
>>>> longOpts, true);
>>>>
>>>> CLHSDB.main(newArgs.toArray(new String[newArgs.size()]));
>>>> }
>>>>
>>>> private static void runHSDB(String[] oldArgs) {
>>>> String[] longOpts = {"exe=", "core=", "pid="};
>>>> ArrayList<String> newArgs = processOptions(oldArgs,
>>>> longOpts, true);
>>>>
>>>> HSDB.main(newArgs.toArray(new String[newArgs.size()]));
>>>> }
>>>>
>>>> private static void runJSTACK(String[] oldArgs) {
>>>> String[] longOpts = {"exe=", "core=", "pid=", "remote=",
>>>> "mixed", "locks"};
>>>> ArrayList<String> newArgs = processOptions(oldArgs,
>>>> longOpts, false);
>>>>
>>>> JStack jstack = new JStack(false, false);
>>>> jstack.runWithArgs(newArgs.toArray(new
>>>> String[newArgs.size()]));
>>>> }
>>>>
>>>> private static void runJMAP(String[] oldArgs) {
>>>> String[] longOpts = {"exe=", "core=", "pid=", "remote=",
>>>> "heap", "binaryheap", "dumpfile=", "histo",
>>>> "clstats", "finalizerinfo"};
>>>>
>>>> ArrayList<String> newArgs = processOptions(oldArgs,
>>>> longOpts, false);
>>>>
>>>> JMap.main(newArgs.toArray(new String[newArgs.size()]));
>>>> }
>>>>
>>>> private static void runJINFO(String[] oldArgs) {
>>>> String[] longOpts = {"exe=", "core=", "pid=", "remote=",
>>>> "flags", "sysprops"};
>>>> ArrayList<String> newArgs = processOptions(oldArgs,
>>>> longOpts, false);
>>>>
>>>> JInfo.main(newArgs.toArray(new String[newArgs.size()]));
>>>> }
>>>>
>>>> private static void runJSNAP(String[] oldArgs) {
>>>> String[] longOpts = {"exe=", "core=", "pid=", "remote=",
>>>> "all"};
>>>> ArrayList<String> newArgs = processOptions(oldArgs,
>>>> longOpts, false);
>>>> JSnap.main(newArgs.toArray(new String[newArgs.size()]));
>>>> }
>>>>
>>>> private static void runDEBUGD(String[] oldArgs) {
>>>> // By default SA agent classes prefer Windows process
>>>> debugger
>>>> // to windbg debugger. SA expects special properties to be
>>>> set
>>>> // to choose other debuggers. We will set those here before
>>>> // attaching to SA agent.
>>>> System.setProperty("sun.jvm.hotspot.debugger.useWindbgDebugger",
>>>> "true");
>>>>
>>>> String[] longOpts = {"exe=", "core=", "pid=", "serverid="};
>>>> ArrayList<String> newArgs = processOptions(oldArgs,
>>>> longOpts, false);
>>>>
>>>> // delegate to the actual SA debug server.
>>>> sun.jvm.hotspot.DebugServer.main(newArgs.toArray(new
>>>> String[newArgs.size()]));
>>>> }
>>>>
>>>>
>>>> Please, let me know what do you think.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 6/9/19 7:29 PM, Yasumasa Suenaga wrote:
>>>>> Sorry, new webrev is here:
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/
>>>>>
>>>>> Yasumasa
>>>>>
>>>>> 2019年6月10日(月) 11:27 Yasumasa Suenaga<yasuenag at gmail.com>:
>>>>>> PING: Could you review them?
>>>>>>
>>>>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8209790
>>>>>>>>>> CSR:https://bugs.openjdk.java.net/browse/JDK-8224979
>>>>>>>>>> webrev:http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/
>>>>>>>>>>
>>>>>> It is P3 bug, but I want to fix it before JDK 13 RDP 1 if possible.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> 2019年6月5日(水) 14:06 Yasumasa Suenaga<yasuenag at gmail.com>:
>>>>>>> Hi Jc,
>>>>>>>
>>>>>>> Thank you for your comment!
>>>>>>> I updated a webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/
>>>>>>>
>>>>>>>> - In runTests; if DebugdUtils implemented Closeable, you
>>>>>>>> could just do a try-with-resources instead of the finally
>>>>>>>> clause...
>>>>>>> I created DebugdUtils for convenience class for attach - detach
>>>>>>> mechanism of debug server.
>>>>>>> IMHO it is prefer "detach" to "close" in this case.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> 2019年6月5日(水) 11:34 Jean Christophe Beyler<jcbeyler at google.com>:
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> I'm not an official reviewer but I don't see an issue with the
>>>>>>>> CSR (except that this seems to be bringing a fork in the tools
>>>>>>>> with some handling remote and others not).
>>>>>>>>
>>>>>>>> However, this code is really repetitive and this is not the
>>>>>>>> place to do a big refactor probably but we could do a few nits
>>>>>>>> perhaps:
>>>>>>>> - Instead of every tool calling commonHelp with an
>>>>>>>> additional flag you could divide into commonHelp and
>>>>>>>> commonHelpWithRemote for the tools and they both call the
>>>>>>>> current commonHelp with that boolean; so that when we are
>>>>>>>> looking at the tool code we know what we are getting... So
>>>>>>>> something like:
>>>>>>>>
>>>>>>>> private static boolean commonHelp(String mode, boolean
>>>>>>>> canConnectToRemote) {
>>>>>>>> ..
>>>>>>>> }
>>>>>>>>
>>>>>>>> private static boolean commonHelp(String mode) {
>>>>>>>> return commonHelp(mode, false);
>>>>>>>> }
>>>>>>>>
>>>>>>>> private static boolean commonHelpWithRemote(String mode) {
>>>>>>>> return commonHelp(mode, false);
>>>>>>>> }
>>>>>>>>
>>>>>>>> and that way the tools that change are just going from:
>>>>>>>> - return commonHelp("jmap");
>>>>>>>> + return commonHelpWithRemote("jmap");
>>>>>>>>
>>>>>>>> - In the same vein, instead of passing null to the
>>>>>>>> buildAttachArgs; you could make a variable null:
>>>>>>>>
>>>>>>>> - buildAttachArgs(newArgs, pid, exe, core, true);
>>>>>>>> + String noRemote = null;
>>>>>>>> + buildAttachArgs(newArgs, pid, exe, core, noRemote, true);
>>>>>>>>
>>>>>>>>
>>>>>>>> -http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdUtils.java.html
>>>>>>>>
>>>>>>>> Nit: you have empty lines at l64 and l73
>>>>>>>>
>>>>>>>> -http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdConnectTest.java.html
>>>>>>>>
>>>>>>>> Nit : you have an empty line at l110
>>>>>>>>
>>>>>>>> - In runTests; if DebugdUtils implemented Closeable, you
>>>>>>>> could just do a try-with-resources instead of the finally
>>>>>>>> clause...
>>>>>>>>
>>>>>>>> All of these are details, I just thought I'd mention them :)
>>>>>>>> Jc
>>>>>>>>
>>>>>>>> On Tue, Jun 4, 2019 at 6:44 PM Yasumasa
>>>>>>>> Suenaga<yasuenag at gmail.com> wrote:
>>>>>>>>> PING: Could you review them?
>>>>>>>>>
>>>>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8209790
>>>>>>>>>> CSR:https://bugs.openjdk.java.net/browse/JDK-8224979
>>>>>>>>>> webrev:http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/
>>>>>>>>>>
>>>>>>>>> CSR status is provisional. So I need reviewers both CSR and
>>>>>>>>> webrev.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 2019年5月29日(水) 22:37 Yasumasa Suenaga<yasuenag at gmail.com>:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Please review this change:
>>>>>>>>>>
>>>>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8209790
>>>>>>>>>> CSR:https://bugs.openjdk.java.net/browse/JDK-8224979
>>>>>>>>>> webrev:http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> In JDK 8 or earlier, some tools (jstack, jmap, jinfo) can
>>>>>>>>>> connect to
>>>>>>>>>> debug server (jsadebugd). However it has not done so since
>>>>>>>>>> JDK 9 because
>>>>>>>>>> jhsdb cannot accept the attach request to debug server.
>>>>>>>>>> So I want to introduce new option `--remote` to connect to
>>>>>>>>>> debug server.
>>>>>>>>>>
>>>>>>>>>> I created CSR for this issue. So please review it together.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jc
>>>>
>>
More information about the serviceability-dev
mailing list