mmtk-core icon indicating copy to clipboard operation
mmtk-core copied to clipboard

Refactoring MMTK instantiation

Open wks opened this issue 3 years ago • 2 comments

Problem

Currently, the MMTK instance is usually created statically.

The practice of using static singletons is https://github.com/mmtk/mmtk-core/issues/58brought from Java JikesRVM, but this is not idiomatic in Rust. Such practice forced two-phase initialisation and forced bad and unsafe practices, including:

  • [X] Using Option<T> for late-initialised fields, and force casting & to &mut to initialise shared data.
    • This has been addressed in issue https://github.com/mmtk/mmtk-core/issues/532, and fixed in https://github.com/mmtk/mmtk-core/pull/539
  • [X] MMTK::options is an UnsafeOptionsWrapper, allowing unsafe update to the shared "supposedly-immutable" MMTK singleton.
    • Fixed in https://github.com/mmtk/mmtk-core/pull/625 by introducing a mutable MMTKBuilder that creates an MMTK instance with immutable options field.
  • [X] There are objects that need to be created/initialised before the MMTK instance is created, such as SFTMap, VMMap and Mmapper. Making MMTK a static singleton makes such dependencies hard to manage.
    • Now those objects are still global singletons implemented as static of lazy_static!, but MMTK is created after them. Therefore the dependencies are clearer.

Related issues:

  • [ ] https://github.com/mmtk/mmtk-core/issues/57
  • [X] https://github.com/mmtk/mmtk-core/issues/58
  • [ ] https://github.com/mmtk/mmtk-core/issues/100

Proposal

Introduce a MMTKBuilder type that builds the MMTK instance.

Following the convention of some builder types in the Rust standard library, such as std::thread::Builder and std::fs:DirBuilder, the MMTKBuilder should be used like the following:

let mmtk = MMTKBuilder::new()
    .heap_size(256 * 1024 * 1024)
    .threads(8)
    .create();

The MMTKBuilder should contain mutable fields so that the user can specify options gradually. But once the MMTK instance is created, the options will be read-only. This means UnsafeOptionWrapper will no longer be necessary.

By the time MMTKBuilder.create() is called, the options should contain all relevant information to create and initialise plans. Because of this, the gc_init methods of many types (such as Plan, Space, etc.) may no longer be necessary, and can be replaced by a new method.

Q/A

Could options be changed at run time? If any options can be changed at run time, it will need proper synchronisation, and the changes to such options must be timely notified to related components. For example, if it is allowed to adjust the number of GC threads at run time, the GC controller thread should be notified about this change, and spawn/kill threads at appropriate time.

wks avatar Feb 07 '22 03:02 wks

This sounds good. And we have multiple issues tracking this or related issues:

  • https://github.com/mmtk/mmtk-core/issues/57
  • https://github.com/mmtk/mmtk-core/issues/58
  • https://github.com/mmtk/mmtk-core/issues/100

Here are some of my thoughts:

  • We probably need to be careful about JikesRVM. They may assume MMTk is statically created, and options are later initialized one by one. We need to be able to deal with VMs like JikesRVM.
  • I think there are third party crates that helps creating builders and command line options. We can consider using them.
  • We may eventually handle deconstruction of an MMTk instance. But that is not the priority for now.
  • About unsafe code: eliminating unsafe code is not a goal for us, but we try to confine its scope, use fewer of them and make sure our use of unsafe code is sound.
  • This issue is probably a big task. We can do small steps each time.

qinsoon avatar Feb 07 '22 04:02 qinsoon

Could options be changed at run time? If any options can be changed at run time, it will need proper synchronisation, and the changes to such options must be timely notified to related components. For example, if it is allowed to adjust the number of GC threads at run time, the GC controller thread should be notified about this change, and spawn/kill threads at appropriate time.

PS I think this should be allowed (at least for certain options -- maybe not all). I ran into this while I was working on the nursery space refactoring. In the original JikesRVM, MMTk temporarily sets the "force full heap gc" to true to force generational plans to perform a full heap GC in harness_begin(). This is missing in Rust MMTk and hence we don't perform a full heap GC for generational GCs in harness_begin().

k-sareen avatar Jul 14 '22 02:07 k-sareen

With MMTKBuilder added, we solved the problems related to Options and the UnsafeOptionsWrapper. With another refactoring that moves global states from BasePlan to a dedicated GlobalStates struct, we have a better place to store mutable global states.

We still have not managed to allow multiple MMTk instances in one process. Let's track that in a different issue, and close this one.

VMMap is a different thing. It basically manages memory at chunk granularity, and we have another issue for refactoring that part. https://github.com/mmtk/mmtk-core/issues/932

wks avatar Nov 22 '23 07:11 wks