Add MMTKBuilder and allow all options to be set from command line #625
Conversation
|
binding-refs |
qinsoon
changed the title
There was a problem hiding this comment.
Having MMTKBuilder to enable safe instantiation of MMTK is a significant improvement.
The main problem of this PR, I think, is the handling of Options. We previously used UnsafeCell because we update the options after MMTK is instantiated. With the builder introduced, it is safe to update options before instantiating MMTK. It is no longer necessary to use UnsafeCell.
src/memory_manager.rs
Outdated
| pub fn process<VM: VMBinding>(mmtk: &'static MMTK<VM>, name: &str, value: &str) -> bool { | ||
| unsafe { mmtk.options.process(name, value) } | ||
| pub fn process(builder: &MMTKBuilder, name: &str, value: &str) -> bool { | ||
| unsafe { builder.set_option(name, value) } |
There was a problem hiding this comment.
If we remove UnsafeOptionsWrapper, this can be made safe. But the builder parameter will need to be &mut, which is reasonable because the MMTk instance has not been created, yet.
I suggest the mmtk-core define process and process_bulk that accept &mut MMTKBuilder. The binding should initialise MMTk using a single thread, or use Mutex<MMTKBuilder>.
There was a problem hiding this comment.
I have made this change as well. I am using Mutex<MMTKBuilder>. As we still use lazy static variables for MMTK and we need the builder to initialize MMTK, we cannot have a non-static MMTKBuilder. We may need to find a better pattern for this.
examples/allocation_benchmark.c
Outdated
| mmtk_set_heap_size(1024*1024*1024); | ||
| mmtk_gc_init(); |
There was a problem hiding this comment.
The mmtk_set_heap_size in the example may give the reader a false impression that the heap size can be changed during execution, which is not true. Without a concrete implementation as in dummyvm, the reader may not realise that it sets an option in the builder before creating an MMTk instance.
For the purpose of example, I think it is OK to keep the old code mmtk_gc_init(1024 * 1024 * 1024); because the C API doesn't have to correspond one-to-one with what memory_manager provides. Conceptually, a C function mmtk_gc_init could both set the option and create the instance. We can also change the name to mmtk_init or init_mmtk to avoid confusion.
There was a problem hiding this comment.
Done. I changed it to mmtk_init(heap_size) (gc_init() is renamed to mmtk_init() -- I think it is a good idea, as it avoids the confusion with initialize_collection()). As for the C API, in this PR, I moved examples/mmtk.h to docs/header/mmtk.h. This is meant to be the canonical header, and the C signatures in the header tries to be one-to-one mapped from the Rust API. The DummyVM binding uses a header in vmbindings/dummyvm/api/mmtk.h, which is different from the canonical version.
examples/allocation_benchmark.c
Outdated
| @@ -5,7 +5,7 @@ | |||
|
|
|||
| int main() { | |||
| volatile uint64_t * tmp; | |||
| mmtk_gc_init(1024*1024*1024); | |||
| mmtk_init(24*1024*1024); | |||
There was a problem hiding this comment.
This is 24 (twenty four) megabytes, but the loop below attempted to allocate 800 megabytes of objects. Is this intentional? Did you mean 1024*1024*1024?
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.
MMTKBuildergc_init()now takes a reference toMMTKBuilderas its argument, and returns a boxed pointer to the new MMTk instance created.gc_init()is renamed tommtk_init().gc_init()no longer requires aheap_sizeargument.heap_sizenow is an option.plan,threads,vm_space_sizeto be set by command lineexamples/mmtk.htodocs/header/mmtk.h. Building C code inexamplesno longer uses this header file.Related PRs: