On Fri, 26 Feb 2021 00:03:40 GMT, Yumin Qi <mi...@openjdk.org> wrote:
> Hi, Please review > > Added jcmd option for dumping CDS archive during application runtime. > Before this change, user has to dump shared archive in two steps: first run > application with > `java -XX:DumpLoadedClassList=<classlist> .... ` > to collect shareable class names and saved in file `<classlist>` , then > `java -Xshare:dump -XX:SharedClassListFile=<classlist> > -XX:SharedArchiveFile=<archivefile> ...` > With this change, user can use jcmd to dump CDS without going through above > steps. Also user can choose a moment during the app runtime to dump an > archive. > The bug is associated with the CSR: > https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved. > New added jcmd option: > `jcmd <pid or AppName> VM.cds static_dump <filename>` > or > `jcmd <pid or AppName> VM.cds dynamic_dump <filename>` > To dump dynamic archive, requires start app with newly added flag > `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to > dynamic dump like loader constraints will be recorded. Note the dumping > process changed some object memory locations so for dumping dynamic archive, > can only done once for a running app. For static dump, user can dump multiple > times against same process. > The file name is optional, if the file name is not supplied, the file name > will take format of `java_pid<number>_static.jsa` or > `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The > `<number>` is the application process ID. > > Tests: tier1,tier2,tier3,tier4 > > Thanks > Yumin Hi Yumin, This is a very useful addition. My biggest concern with this patch though is the use of `os::fork_and_exec()` in regular, non-fatal situations. I had a look at that function and I think that is very unsafe. - it does not close file descriptors, so the child vm will inherit all file descriptors not opened with FD_CLOEXEC. In Runtime.exec, we do this whole dance around safely closing off all unused file descriptors, which is missing here. Note that even though we mostly open all fds with CLOEXEC, this does not matter, since user code may not do that. - It uses vfork "if available", which is probably always, but that may be okay since the child exec's right away. Still, vfork makes me nervous. - Weirdly enough, it always spawns the child program via one indirection using a shell; so there is always the shell between you and your spawned VM, and you probably wont get the return code of your child vm process back. Until now, os::fork_and_exec() was only used in fatal situations where the VM was about to die anyway. And where it did not really matter if it worked or not. If we now want to use it in regular situations, we need to give that thing an overhaul and make it a first class fork api, and also test it better that it is today. The file descriptor issue has to be addressed at the very least. I really would consider rewriting the whole thing using posix_spawn. Since JDK15 I think posix_spawn is the default for Runtime.exec, so we know it works well. I would also do this in a separate RFE. Alternatively, you could call into java and use Runtime.exec(). Further remarks inline. Thanks, Thomas src/hotspot/share/memory/metaspaceShared.cpp line 883: > 881: snprintf(buff_start, sizeof(exec_path), " -cp %s %s", app_class_path, > java_command); > 882: output->print_cr("%s", exec_path); > 883: os::fork_and_exec(exec_path); Apart from what I wrote above, how do you handle fork/exec errors? There are a number of things that can go wrong here. At the very least I would scan the return code. And the return code of the child VM. ------------- Changes requested by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2737
