Add a simple implementation for find_object_from_internal_pointer #1155
Conversation
| } else { | ||
| None | ||
| } | ||
| pub fn is_vo_bit_set_for_addr<VM: VMBinding>(address: Address) -> bool { |
There was a problem hiding this comment.
This function used to return ObjectReference. However we cannot be sure that the ObjectReference is actually valid. I just change it to return boolean, and let the caller figure out the object reference.
| /// the binding may have internal pointers on the stack. | ||
| /// | ||
| /// The function cannot directly return an object reference. Instead, it returns | ||
| /// an address range and guarantees the object ference is in the range. |
There was a problem hiding this comment.
| /// an address range and guarantees the object ference is in the range. | |
| /// an address range and guarantees the object reference is in the range. |
| /// bit to represent a valid object for every 8 bytes ([`crate::util::is_mmtk_object::VO_BIT_REGION_SIZE`]). | ||
| /// We use VO bits to find the object for an internal pointer. | ||
| /// When we find a set VO bit, we only know that in the 8 bytes there is an object, and we cannot know where | ||
| /// exactly the object is. The binding needs to use their knowledge about the alignment |
There was a problem hiding this comment.
| /// exactly the object is. The binding needs to use their knowledge about the alignment | |
| /// exactly the object is. The binding needs to use its knowledge about the alignment |
| pub fn find_object_from_internal_pointer<VM: VMBinding>( | ||
| internal_ptr: Address, | ||
| max_search_bytes: usize, | ||
| ) -> Option<(Address, Address)> { |
There was a problem hiding this comment.
We may simply return Option<Address> because the length of the range is always the "bytes in region" of the VO bit metadata. It is conveniently defined as mmtk::util::is_mmtk_object::VO_BIT_REGION_SIZE.
| ) -> Option<(Address, Address)> { | |
| ) -> Option<Address> { |
There was a problem hiding this comment.
Option<Address> is unfortunately larger than usize. Maybe we need to revisit Address and make it non-nullable as well to make Option<Address> easier to use as well as make it more idiomatic Rust
|
|
||
| let addr = potential_object.to_address::<VM>(); | ||
| fn _is_vo_bit_set<const ATOMIC: bool, VM: VMBinding>(address: Address) -> bool { |
There was a problem hiding this comment.
Rust has the pub keyword, so anything without a pub is already private. We don't need add an underscore explicitly.
| fn _is_vo_bit_set<const ATOMIC: bool, VM: VMBinding>(address: Address) -> bool { | |
| fn is_vo_bit_set<const ATOMIC: bool, VM: VMBinding>(address: Address) -> bool { |
| if let Some(potential_object) = ObjectReference::from_raw_address(address) { | ||
| let addr = potential_object.to_address::<VM>(); |
There was a problem hiding this comment.
I see this is a bit complicated.
Would it make things easier if we change the contract of is_mmtk_object(addr) so that addr must always be 8 byte aligned (or whatever the byte-per-region of VO_BIT is)? In this case, we always check if a VO bit is set at address addr. But when the VM binding does conservative stack scanning, it needs to decide which address to query according to its offset of ref_to_address.
I have already thought about is_mmtk_object being unsound in the sense that the argument addr is not always a valid object reference. Maybe we just provide a safe variant of is_vo_bit_set that checks against the SFT and the memory mapping for the VM binding. And we explicitly tell the VM that the VO bit is set at objref.to_address() rounded down to a multiple of 8 bytes, and let the VM binding figure out how to use it wisely.
There was a problem hiding this comment.
Would it make things easier if we change the contract of
is_mmtk_object(addr)so thataddrmust always be 8 byte aligned (or whatever the byte-per-region ofVO_BITis)? In this case, we always check if a VO bit is set at addressaddr. But when the VM binding does conservative stack scanning, it needs to decide which address to query according to its offset ofref_to_address.
That will be annoying to the bindings, and expose unnecessary internal details.
I think our current code tries to be unnecessarily general. This results in more bugs, complex code and inefficiency. I would rather introduce some sane assumptions to make both our API and the internal implementation simpler.
There was a problem hiding this comment.
We already allows the VM binding to set the VO bit in the allocation fast path which inlines the post_alloc function. So the VM binding already knows where VO bit should be set. So changing the contract to explicitly mention VO bit does not add more burden to the VM binding.
There was a problem hiding this comment.
We already allows the VM binding to set the VO bit in the allocation fast path which inlines the
post_allocfunction. So the VM binding already knows where VO bit should be set. So changing the contract to explicitly mention VO bit does not add more burden to the VM binding.
What you said is an optimization. It just copies what we do in mmtk-core, so it naturally assumes our internal implementation. But what we discussed is the API -- it should not expose the internal implementation where possible.
|
Other than the inefficiency, one big issue for this PR is that the address we return may not be the base reference for the given address. It could be the previous object that ends before the address. We will have to check against the object size to know if the given address is within the object range. However, we cannot get the object size as we don't know the object reference. I think we would want to enforce object reference alignment so we can know the object reference using VO bit. I feel that is essential. |
This PR adds internal pointer support. It supersedes #1155 which provides a simple but inefficient implementation for internal pointers. This PR is based on #1159 which adds requirements for object reference alignment. This PR * adds `memory_manager::find_object_from_internal_pointer` * The call is dispatched using SFT to each space. * Large object space only checks the first word in VO bit for every page. * Mark sweep and immix space only searches for the max object size for those spaces. * Allow iterating side metadata bits. * Allow loading raw byte/word in side metadata.
|
Superseded by #1165 |
This PR adds a function to our API
find_object_from_internal_pointer. It uses a simple implementation withis_vo_bit_set_for_addrwhich is not efficient.An efficient implementation should be able to bulk load side metadata, check the last bit set and deduce the data address from the bit. However, there are many corner cases for this that I haven't sorted out, and it would need more testing before we can make it ready. I think it might be a good idea to have the API first with a naive implementation which works. We can optimize it later.