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

Let ObjectReference implement Ord #870

Merged
merged 1 commit into from
Aug 3, 2023
Merged

Let ObjectReference implement Ord #870

merged 1 commit into from
Aug 3, 2023

Conversation

wks
Copy link
Collaborator

@wks wks commented Jul 28, 2023

This will enable Vec<ObjectReference> to be sorted, and enable ObjectReference to be used as the keys of ordered collections such as BTreeMap. Although mmtk-core currently never sorts Vec<ObjectReference>, developers of mmtk-core as well as bindings may experiment with accessing ObjectReference instances from low addresses to high addresses. It complicates things if ObjectReference does not implement Ord.

@wks wks requested a review from qinsoon July 28, 2023 06:13
@caizixian
Copy link
Member

I'm ok with this PR, but I think there's a separate problem we need to rethink. On a conceptual level (which is not the current implenmentation afaik), object references could be some arbitrary handles that is supposed to be opaque to mmtk-core, and you use it by calling the appropriate methods exposed by the VM object model and scanning code. In that sense, the core may or may not be able to depend on the ordering of the handles existing, nor does the ordering always make sense (e.g. if the core starts to depend on the ordering for locality).

@wks
Copy link
Collaborator Author

wks commented Aug 1, 2023

I'm ok with this PR, but I think there's a separate problem we need to rethink. On a conceptual level (which is not the current implenmentation afaik), object references could be some arbitrary handles that is supposed to be opaque to mmtk-core, and you use it by calling the appropriate methods exposed by the VM object model and scanning code. In that sense, the core may or may not be able to depend on the ordering of the handles existing, nor does the ordering always make sense (e.g. if the core starts to depend on the ordering for locality).

@caizixian We discussed handles before. The conclusion is, if a VM uses indirection tables, and handles are pointers (or indices) of the indirection table entries, then the binding only needs to give MMTk-core the contents of the indirection tables (i.e. the pointer to the object), and allows MMTk-core to update the contents of the indirection tables. The binding doesn't need to give the handles themselves to the MMTk core, and MMTk core shouldn't (and in fact cannot) create handles. So in the end, we define ObjectReference to be a pointer to an object, although it may point to the beginning of an object or an offset from the beginning.

Details can be found in the following Zulip channels:

@caizixian
Copy link
Member

Right. Thanks. I have forgotten. We should collate our design documentation and decisions in one place at some point :)

@qinsoon
Copy link
Member

qinsoon commented Aug 1, 2023

Right. Thanks. I have forgotten. We should collate our design documentation and decisions in one place at some point :)

It is stated in the comments:

/// A runtime may define its "object references" differently. It may define an object reference as
/// the address of an object, a handle that points to an indirection table entry where a pointer to
/// the object is held, or anything else. Regardless, MMTk expects each object reference to have a
/// pointer to the object (an address) in each object reference, and that address should be used
/// for this `ObjectReference` type.

@qinsoon
Copy link
Member

qinsoon commented Aug 1, 2023

However, I feel it a bit strange to allow ordering on ObjectReference. In what case would object references need to be sorted? In your experiment before, sorting object references did not improvement performance, as the cost of sorting overweighs the benefits of locality. Is there any real need that we know that we want to sort on ObjectReference?

@wks
Copy link
Collaborator Author

wks commented Aug 2, 2023

However, I feel it a bit strange to allow ordering on ObjectReference. In what case would object references need to be sorted? In your experiment before, sorting object references did not improvement performance, as the cost of sorting overweighs the benefits of locality. Is there any real need that we know that we want to sort on ObjectReference?

I haven't found any case where sorting ObjectReference is definitely beneficial. But experiments are still going on. At this stage, the benefit of letting ObjectReference implement Ord is merely making experiments easier.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize we have PartialOrd on ObjectReference. If we have PartialOrd, it makes sense to have Ord.

But I would think we should have none on them. The current MMTk core code does not rely on it either. It is not intuitive to compare the order of two object references. We can compare the raw address for object references, something like vec.sort_by(|obj1, obj2| obj1.to_raw_address().cmp(&obj2.to_raw_address())) works well.

@caizixian
Copy link
Member

something like vec.sort_by(|obj1, obj2| obj1.to_raw_address().cmp(&obj2.to_raw_address())) works well.

I think one of Kunshan's motivations is to use data structures like BTreeMap. If we don't have ordering, then we limit ourselves to only hashing-based data structures.

@caizixian caizixian merged commit 5d22c2b into mmtk:master Aug 3, 2023
13 of 14 checks passed
@k-sareen
Copy link
Collaborator

k-sareen commented Aug 3, 2023

Yeah but we could order the Addresses instead of ObjectReferences. Address has a definitive order, I'm not sure about ObjectReference given we want to treat them as opaque to MMTk.

caizixian added a commit to caizixian/mmtk-core that referenced this pull request Aug 3, 2023
Co-authored-by: Claire Huang <claire.x.huang@gmail.com>

Use probe 0.3 for MSRV

Documentation

Address review comments

Update src/util/alloc/allocator.rs

Co-authored-by: Kunal Sareen <kunal.sareen@anu.edu.au>

clippy fix

Let ObjectReference implement Ord (mmtk#870)

This will enable `Vec<ObjectReference>` to be sorted, and enable
`ObjectReference` to be used as the keys of ordered collections such as
`BTreeMap`. Although mmtk-core currently never sorts
`Vec<ObjectReference>`, developers of mmtk-core as well as bindings may
experiment with accessing `ObjectReference` instances from low addresses
to high addresses. It complicates things if `ObjectReference` does not
implement `Ord`.

Minor

Add more tracepoints so that tracing scripts don't need to depend on the symbols exported by the binding
caizixian added a commit to caizixian/mmtk-core that referenced this pull request Aug 3, 2023
Co-authored-by: Claire Huang <claire.x.huang@gmail.com>

Use probe 0.3 for MSRV

Documentation

Address review comments

Update src/util/alloc/allocator.rs

Co-authored-by: Kunal Sareen <kunal.sareen@anu.edu.au>

clippy fix

Let ObjectReference implement Ord (mmtk#870)

This will enable `Vec<ObjectReference>` to be sorted, and enable
`ObjectReference` to be used as the keys of ordered collections such as
`BTreeMap`. Although mmtk-core currently never sorts
`Vec<ObjectReference>`, developers of mmtk-core as well as bindings may
experiment with accessing `ObjectReference` instances from low addresses
to high addresses. It complicates things if `ObjectReference` does not
implement `Ord`.

Minor

Add more tracepoints so that tracing scripts don't need to depend on the symbols exported by the binding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants