Thanks,
Yasumasa
Thanks,
Serguei
On 6/16/19 14:44, serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> wrote:
> 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.spit...@oracle.com
<mailto:serguei.spit...@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.spit...@oracle.com
<mailto:serguei.spit...@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<yasue...@gmail.com
<mailto:yasue...@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<yasue...@gmail.com
<mailto:yasue...@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<jcbey...@google.com <mailto:jcbey...@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<yasue...@gmail.com <mailto:yasue...@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<yasue...@gmail.com <mailto:yasue...@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
>>>>>
>>>
>
