Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring MMTK instantiation #541

Closed
5 tasks done
wks opened this issue Feb 7, 2022 · 3 comments
Closed
5 tasks done

Refactoring MMTK instantiation #541

wks opened this issue Feb 7, 2022 · 3 comments
Labels
A-interface Area: Interface/API C-feature Category: Feature P-normal Priority: Normal.

Comments

@wks
Copy link
Collaborator

wks commented Feb 7, 2022

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:

  • Using Option<T> for late-initialised fields, and force casting & to &mut to initialise shared data.
  • MMTK::options is an UnsafeOptionsWrapper, allowing unsafe update to the shared "supposedly-immutable" MMTK singleton.
  • 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.

Sub-issues:

Related issues:

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.

@qinsoon
Copy link
Member

qinsoon commented Feb 7, 2022

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

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.

@k-sareen
Copy link
Collaborator

k-sareen commented Jul 14, 2022

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().

qinsoon added a commit that referenced this issue Jul 20, 2022
This PR adds `MMTKBuilder`, and changes APIs around initializing MMTk. The change allows all the options to be set from command line, including heap size, plan, GC threads, etc.

This PR closes #623. This PR is also an initial step for #541. For #541, we may need further changes.

* Add `MMTKBuilder`
* `gc_init()` now takes a reference to `MMTKBuilder` as its argument, and returns a boxed pointer to the new MMTk instance created.
* `gc_init()` is renamed to `mmtk_init()`.
* `gc_init()` no longer requires a `heap_size` argument. `heap_size` now is an option.
* allow options for `plan`, `threads`, `vm_space_size` to be set by command line
* move `examples/mmtk.h` to `docs/header/mmtk.h`. Building C code in `examples` no longer uses this header file.

Related PRs:
* mmtk/mmtk-openjdk#166
* mmtk/openjdk#12
* mmtk/mmtk-jikesrvm#118
* mmtk/mmtk-v8#64
@k-sareen k-sareen added the P-normal Priority: Normal. label Nov 22, 2023
@wks
Copy link
Collaborator Author

wks commented Nov 22, 2023

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. #932

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-interface Area: Interface/API C-feature Category: Feature P-normal Priority: Normal.
Projects
None yet
Development

No branches or pull requests

3 participants