Mimalloc Allocator #643
Merged
Mimalloc Allocator #643
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wks
reviewed
Nov 11, 2022
Comment on lines
+344
to
+366
| impl<VM: VMBinding> GCWork<VM> for SweepChunk<VM> { | ||
| #[inline] | ||
| fn do_work(&mut self, _worker: &mut GCWorker<VM>, _mmtk: &'static MMTK<VM>) { | ||
| debug_assert!(self.space.chunk_map.get(self.chunk) == ChunkState::Allocated); | ||
| // number of allocated blocks. | ||
| let mut allocated_blocks = 0; | ||
| // Iterate over all allocated blocks in this chunk. | ||
| for block in self | ||
| .chunk | ||
| .iter_region::<Block>() | ||
| .filter(|block| block.get_state() != BlockState::Unallocated) | ||
| { | ||
| if !block.attempt_release(self.space) { | ||
| // Block is live. Increment the allocated block count. | ||
| allocated_blocks += 1; | ||
| } | ||
| } | ||
| // Set this chunk as free if there is not live blocks. | ||
| if allocated_blocks == 0 { | ||
| self.space.chunk_map.set(self.chunk, ChunkState::Free) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I have some radical thoughts about this. Maybe this SweepChunk work packet is completely unnecessary.
If each BlockList is owned by exactly one mutator (or the global MarkSweepSpace), and each Block is owned by exactly one BlockList, then we can traverse all existing blocks simply by traversing all BlockList instances. That means visiting all BlockList in mutators and the lists in MarkSweepSpace::abandoned_lists. Doing so allows blocks to be swept while the current thread has exclusive access to its BlockList, therefore removing the need to have lock() in BlockList completely.
But I prefer doing this under the protection of Rust's ownership model. From the current code, it looks like there is no way for Block instances to be created anywhere else (other than MarkSweepSpace::acquire_block), held anywhere else, or discarded before they are swept. Rust's ownership model can enforce that, but currently our Block type implements Copy, has pointer semantics, and bypasses the ownership model. I elaborated my thoughts here: #696.
qinsoon
added a commit
that referenced
this pull request
Nov 25, 2022
This PR removes most of our uses of `ObjectReference::to_address()`, and replace that with `ObjectModel::ref_to_address()`. With this change, the address we get from an object reference is always guaranteed to be within the allocation. Thus changes in #555 are reverted in this PR. This PR also addresses the comments raised in #643 (comment). Changes: * Replace our use of `ObjectReference::to_address()` to `ObjectModel::ref_to_address()` * Changes to `ObjectReference`: * Rename `ObjectReference::to_address()` to `ObjectReference::to_raw_address()`, and make it clear that we should avoid using it. * Remove `Address::to_object_reference()`, and add `ObjectReference::from_raw_address()`. Make it clear that we should avoid using it. * Changes to `ObjectModel`: * add `address_to_ref` which does the opposite of `ref_to_address` * add `ref_to_header` * rename `object_start_ref` to `ref_to_object_start`, to make it consistent with other methods. * Change `Treadmill` to store `ObjectReference` instead of `Address`. We previously stored object start address in `Treadmill` and we assumed alloc bit is set on the object start address. With changes in this PR, alloc bit is no longer set on object start address. I made changes accordingly. * Remove code about 'object ref guard' which was used to deal with the case that an object reference (address) may not be in the same chunk as the allocation. That should no longer happen. * `alloc_bit::is_alloced_object(address)` now returns an `Option<ObjectReference>`. We may consider return `Option<ObjectReference>` with our API `is_mmtk_object()` as well, but i did not include this change in the PR.
|
I believe I have addressed the issues you pointed out. Can you review the PR again and let me know if there are any further changes required? @wks @wenyuzhao |
wks
requested changes
Nov 28, 2022
wks
approved these changes
Nov 29, 2022
|
It looks good to me now. |
wenyuzhao
approved these changes
Nov 30, 2022
|
I managed to get the mmtk-ruby binding working. However, if I switch from malloc MarkSweep to native MatkSweep, it will crash in I am still investigating. |
|
I think the implementation of
(FYI, for Ruby, I think it is unsound to clear 0 or 1 bytes. It should only clear the bit range the object occupies, which may span multiple bytes. |
wks
requested changes
Nov 30, 2022
qinsoon
added a commit
that referenced
this pull request
Dec 2, 2022
This PR fixes a bug in `bzero_metadata()` discussed in #643 (comment). When the data address range to be bulk zeroed cannot be mapped into whole bytes in the side metadata, the other bits in the bytes which should not be updated will get zeroed unexpectedly.
|
binding-refs |
udesou
pushed a commit
to udesou/mmtk-core
that referenced
this pull request
Dec 5, 2022
This PR fixes a bug in `bzero_metadata()` discussed in mmtk#643 (comment). When the data address range to be bulk zeroed cannot be mapped into whole bytes in the side metadata, the other bits in the bytes which should not be updated will get zeroed unexpectedly.
wenyuzhao
pushed a commit
to wenyuzhao/mmtk-core
that referenced
this pull request
Mar 20, 2023
This PR addresses the issues we discussed in mmtk#643 (comment). Basically, Rust expects `From<T>` to always succeed, and Rust suggests using `TryFrom<T>` if the conversion may fail. For us, turning an address into a region may fail. So we should not use `From<T>`. And we do not need the error handling with `TryFrom<T>`. Thus we just provide two methods: `Region::from_unaligned_address()` and `Region::from_aligned_address()`.
wenyuzhao
pushed a commit
to wenyuzhao/mmtk-core
that referenced
this pull request
Mar 20, 2023
This PR removes most of our uses of `ObjectReference::to_address()`, and replace that with `ObjectModel::ref_to_address()`. With this change, the address we get from an object reference is always guaranteed to be within the allocation. Thus changes in mmtk#555 are reverted in this PR. This PR also addresses the comments raised in mmtk#643 (comment). Changes: * Replace our use of `ObjectReference::to_address()` to `ObjectModel::ref_to_address()` * Changes to `ObjectReference`: * Rename `ObjectReference::to_address()` to `ObjectReference::to_raw_address()`, and make it clear that we should avoid using it. * Remove `Address::to_object_reference()`, and add `ObjectReference::from_raw_address()`. Make it clear that we should avoid using it. * Changes to `ObjectModel`: * add `address_to_ref` which does the opposite of `ref_to_address` * add `ref_to_header` * rename `object_start_ref` to `ref_to_object_start`, to make it consistent with other methods. * Change `Treadmill` to store `ObjectReference` instead of `Address`. We previously stored object start address in `Treadmill` and we assumed alloc bit is set on the object start address. With changes in this PR, alloc bit is no longer set on object start address. I made changes accordingly. * Remove code about 'object ref guard' which was used to deal with the case that an object reference (address) may not be in the same chunk as the allocation. That should no longer happen. * `alloc_bit::is_alloced_object(address)` now returns an `Option<ObjectReference>`. We may consider return `Option<ObjectReference>` with our API `is_mmtk_object()` as well, but i did not include this change in the PR.
wenyuzhao
pushed a commit
to wenyuzhao/mmtk-core
that referenced
this pull request
Mar 20, 2023
This PR fixes a bug in `bzero_metadata()` discussed in mmtk#643 (comment). When the data address range to be bulk zeroed cannot be mapped into whole bytes in the side metadata, the other bits in the bytes which should not be updated will get zeroed unexpectedly.
wenyuzhao
pushed a commit
to wenyuzhao/mmtk-core
that referenced
this pull request
Mar 20, 2023
This pull request introduces an allocator based on Microsoft's mimalloc (https://www.microsoft.com/en-us/research/uploads/prod/2019/06/mimalloc-tr-v1.pdf) used in a MarkSweep GC. This closes mmtk#348. This PR has a few leftover issues: mmtk#688 General changes in this PR (if requested, I can separate these changes to different PRs): * `destroy_mutator()`: * now takes `&mut Mutator` instead of `Box<Mutator>`. The reclamation of the `Mutator` memory is up to the binding, and allows the binding to copy the `Mutator` value to their thread local data structure, in which case, Rust cannot reclaim the memory as if it is a `Box`. * now calls each allocator for their allocator-specific `on_destroy()` behavior. * Extract `Chunk` and `ChunkMap` from Immix, and make it general for other policies to use. * Changes in `VMBinding` constants: * `LOG_MIN_ALIGNMENT` is removed. We still provide `MIN_ALIGNMENT`. This avoids confusion that a binding may override one constant but not the other. * `MAX_ALIGNMENT_SHIFT` is removed. We provide `MAX_ALIGNMENT` for the same reason as above. * Add `USE_ALLOCATION_OFFSET: bool`. If a binding never use allocation offset (i.e. `offset === 0`), they can set this to `false`, and MMTk core could use this for some optimizations. * Changes in `ObjectModel`: * Add `UNIFIED_OBJECT_REFERENCE_ADDRESS: bool` to allow a binding to tell us if the object reference uses the same address as its object start and its to_address. * Add `OBJECT_REF_OFFSET_LOWER_BOUND` to allow binding to tell us roughly where the object reference points to (with respect of the allocation address). * Changes to related methods to cope with this change. Mark sweep changes in this PR: * add two features for marksweep * `eager_sweeping`: sweep unused memory eagerly in each GC. Without this feature, unused memory is swept when we attempt to allocate from them. * `malloc_mark_sweep`: the same as our previous `malloc_mark_sweep`. When this is used, the mark sweep plan uses `MallocSpace` and `MallocAllocator`. * Move the current `policy::mallocspace` to `policy::marksweepspace::malloc_ms` * Add `policy::marksweepspace::native_ms` for the mark sweep space with mimalloc. * Add `util::alloc::free_list_allocator`. Co-authored-by: Yi Lin <qinsoon@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces an allocator based on Microsoft's mimalloc (https://www.microsoft.com/en-us/research/uploads/prod/2019/06/mimalloc-tr-v1.pdf) used in a MarkSweep GC. This closes #348.
This PR has a few leftover issues: #688
General changes in this PR (if requested, I can separate these changes to different PRs):
destroy_mutator():&mut Mutatorinstead ofBox<Mutator>. The reclamation of theMutatormemory is up to the binding, and allows the binding to copy theMutatorvalue to their thread local data structure, in which case, Rust cannot reclaim the memory as if it is aBox.on_destroy()behavior.ChunkandChunkMapfrom Immix, and make it general for other policies to use.VMBindingconstants:LOG_MIN_ALIGNMENTis removed. We still provideMIN_ALIGNMENT. This avoids confusion that a binding may override one constant but not the other.MAX_ALIGNMENT_SHIFTis removed. We provideMAX_ALIGNMENTfor the same reason as above.USE_ALLOCATION_OFFSET: bool. If a binding never use allocation offset (i.e.offset === 0), they can set this tofalse, and MMTk core could use this for some optimizations.ObjectModel:UNIFIED_OBJECT_REFERENCE_ADDRESS: boolto allow a binding to tell us if the object reference uses the same address as its object start and its to_address.OBJECT_REF_OFFSET_LOWER_BOUNDto allow binding to tell us roughly where the object reference points to (with respect of the allocation address).Mark sweep changes in this PR:
eager_sweeping: sweep unused memory eagerly in each GC. Without this feature, unused memory is swept when we attempt to allocate from them.malloc_mark_sweep: the same as our previousmalloc_mark_sweep. When this is used, the mark sweep plan usesMallocSpaceandMallocAllocator.policy::mallocspacetopolicy::marksweepspace::malloc_mspolicy::marksweepspace::native_msfor the mark sweep space with mimalloc.util::alloc::free_list_allocator.