[crac] RFR: Allow users to pass new properties on restore

[Anton Kozlov]

On Thu, 14 Apr 2022 15:07:22 GMT, Ashutosh Mehra <duke at openjdk.java.net> wrote:

> VM changes: To identify properties that can be modified on restore,
> added a new bool field SystemProperty::_modifiable_on_restore.
> All the jdk related properties are marked unmodifiable. Rest of the
> properties are considered modifiable.
> When the JVM is launched with -XX:CRaCRestoreFrom option, then the
> properties prefixed with "-D" are maintained in a separate list in
> Arguments::_system_properties_for_restore. This list is passed to the JVM
> being restored by writing to a shared memory object.
> When the JVM is restored, it reads the new properties from shared memory
> object and updates its existing list of properties maintained in
> Arguments::_system_properties.
> 
> JDK changes: System::props needs to be updated on restore to account for
> new system properties. For this purpose j.l.System registers a new
> JDKResource which queries new properties from the VM in afterRestore()
> notification and updates System::props. The JDKResource registered by
> j.l.System is given highest priority so it is the first resource to get
> afterRestore() notification.
> 
> Signed-off-by: Ashutosh Mehra <asmehra at redhat.com>

> VM changes: To identify properties that can be modified on restore, added a new bool field SystemProperty::_modifiable_on_restore. All the jdk-related properties are marked unmodifiable. The rest of the properties are considered modifiable.

On the java level, all properties can be changed with `System.setProperty`. Our property model is much closer to a program do e.g. after restore

        System.setProperty("sun.boot.library.path", "test");

There is no mechanism (except the deprecated SecurityManager) that prevents this from working, although no one promises the property value will be considered if set in this way. So I propose not to limit ourselves artificially. Who knows, maybe some JDK system property is OK to fix it on restore, e.g. if the checkpoint is done very early and a class that reads the property is not yet initialized.

> When the JVM is launched with -XX:CRaCRestoreFrom option, then the properties prefixed with "-D" are maintained in a separate list in Arguments::_system_properties_for_restore.

Have you considered how to make this change smaller? E.g adding an Explicit marker on the property? That's unfortunate that implicit properties are already added to the list before we get to the restore, and explicit properties cannot be distinguished from them. I'm fine with the change in Arguments::parse_each_vm_init_arg, but refactorings or adding a set of methods to manage the for_restore set is a bit overkill. Initializing a full VM just to be replaced by another restored one (as we do for now) does not make a lot of sense and needs fixing at some moment. So I'd like to avoid too many changes in the VM code that we'll have to revert.

Another way is to pass all properties from restoring VM to the one being restored, for simplicity and flexibility. And to filter properties in the being restored VM -- that will be the single point of responsibility. But here there is a drawback: VMs may have different versions, so implicit properties in the being restored VM will be implicitly overwritten.

> This list is passed to the JVM being restored by writing to a shared memory object. When the JVM is restored, it reads the new properties from shared memory object and updates its existing list of properties maintained in Arguments::_system_properties.

I could not find users of the _system_properties after VM was initialized and JDK pulled the set of properties from the VM. I think it's not necessary to do, just as a call to j.l.System.setProperty is not reflected in the VM.

> JDK changes: System::props needs to be updated on restore to account for new system properties. For this purpose j.l.System registers a new JDKResource which queries new properties from the VM in afterRestore() notification and updates System::props. The JDKResource registered by j.l.System is given highest priority so it is the first resource to get afterRestore() notification.

AFAIU, for the regular bootstrap procedure, "pulling" of properties is required as the VM does not know when j.l.System will be initialized, so instead, they are pulled at the j.l.System's initialization. But I think we can assume j.l.System is always initialized at the checkpoint, so we can avoid the resource, and make jdk.crac.Core just to set the new properties with the j.l.System.setProperty.

src/hotspot/os/linux/os_linux.cpp line 5858:

> 5856:   }
> 5857: 
> 5858:   write(shmfd, (void *)&props_count, sizeof(props_count));

This seems to write a header for the subsequent data. Could you create a `struct` for that so the format will be explicitly written?

src/hotspot/share/prims/jvm.cpp line 322:

> 320:  * names and values from the jvm SystemProperty which are modifiable on restore.
> 321:  */
> 322: JVM_ENTRY(jobjectArray, JVM_GetModifiableProperties(JNIEnv *env))

Instead of a new JVM interface and JNI function, please just return the properties object along the rest of information from os::Linux::checkpoint

src/java.base/share/classes/java/lang/System.java line 2110:

> 2108: 
> 2109:     private static void registerCRaCResource() {
> 2110:         jdk.internal.crac.Core.getJDKContext().register(new CRaCResource());

The weak link to the CRaCResource object won't prevent the object to become unreachable and thus properties won't be updated, e.g. after a few GCs.

-------------

PR: https://git.openjdk.java.net/crac/pull/21