On Tue, 9 Feb 2021 19:13:55 GMT, Chris Plummer <[email protected]> wrote:
>>> Hi @Hamlin-Li, >>> >>> It looks good in general. >>> A question: What is going to happen if the number of parallel threads >>> passed to the command is too big? Is there any limit, and what has to be >>> the expected behavior in such a case? I'll look at the CSR. >>> Thanks, >>> Serguei >> >> Hi Serguer, >> Thanks for reviewing. >> First, it got truncated as uint by ClassHistogramDCmd, then it got capped in >> HeapInspection by by total_workers of gang, so finally it will not exceed >> total_workers. >> Hope this answer your questions. > > I think part of the concern here is that this is all hidden from the user. > "total_workers of gang" is not something that is spec'd somewhere. So when > the user chooses to set "parallel" to something other than 0 (the default), > then there really is no guidance as to what a good alternative number is. > Presumably you are setting it because you feel the default is too low or too > high, but without knowing the actual number of threads used by default, it's > hard to come up with a reasonable adjustment to specify. We also have the > relationship to `-XX:ParallelGCThreads`, which impacts this. > > Maybe the option should just be to enable or disable parallel. Before your > changes it looks like you got parallel=1, so no parallel threads, and no > ability to change that. With your changes the default is now parallel=0, so > you get the default number of threads, and you can change it to any number. > So maybe we should choose one of the following two simpler approaches: > > - Default to not parallel. Allow the user to specify `-parallel`, but not > specify the thread count. They will get the default number of threads just > like `parallel=0` > - Default to parallel using the default number of threads. Allow the user to > turn parallel off with a `-noparallel` option. > > However, this probably needs to be consistent with the jmap command and the > attach API protocol, which is being discussed in #2261. They should both > either choose one of the above simplified approaches, or do a better job of > specifying defaults and limits for the number of threads. So some input from > @linzang woudl be useful here. Hi Chris, > * Default to not parallel. Allow the user to specify `-parallel`, but not > specify the thread count. They will get the default number of threads just > like `parallel=0` > * Default to parallel using the default number of threads. Allow the user to > turn parallel off with a `-noparallel` option. I would prefer the -noparallel option that enable parallel by default. IMHO, the parallel heap inspection is a little bit like parallelGCThreads - which allowing the hotspot to decide the parallel thread number based on the platfrom by default. And I remember Serguei (@sspitsyn) and I discussed the "-parallel" option at https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-September/032836.html, we finally decide to not introduce it. just FYI. > However, this probably needs to be consistent with the jmap command and the > attach API protocol, which is being discussed in #2261. if just introducing new option like "parallel" or "noparallel", I think we don't need to bother about attach API protocol. we can preprocess it in JMap.java and then pass the value of "0" or "1" as parallel_thread_number, just like what it is implemented at present, no compatibility issue introduced :-) Moreover, I am thinking maybe we could print a meesage like "inspected heap parallelly with `parallel_numer` threads" in the result, just like -dump's output "Heap dump file created [18904360 bytes in 0.039 secs]" Thanks, Lin ------------- PR: https://git.openjdk.java.net/jdk/pull/2379
