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

Require MMTk-level ObjectReference to be inside an object #178

Closed
wks opened this issue Aug 28, 2024 · 0 comments · Fixed by #180
Closed

Require MMTk-level ObjectReference to be inside an object #178

wks opened this issue Aug 28, 2024 · 0 comments · Fixed by #180

Comments

@wks
Copy link
Collaborator

wks commented Aug 28, 2024

Issue mmtk/mmtk-core#1170 proposed requiring the ObjectReference type in mmtk-core to always point into a byte inside an object (in addition to the already-added requirement that ObjectReference must be word-aligned). That will make side metadata handling much easier, and eliminate the need to distinguish between ObjectReference.to_address::<VM>() and ObjectReference.to_raw_address() which is confusing.

JikesRVM is the only officially supported VM where object.to_address::<JikesRVM>() is different from object.to_raw_address(). In JikesRVM, ObjectReference is defined as the address of the array payload. The entire JikesRVM is built around this definition. Constants (such as TIB_OFFSET, STATUS_OFFSET and ARRAY_LENGTH_OFFSET) are all relative to the JikesRVM-level ObjectReference.

However, to implement mmtk/mmtk-core#1170, we must redefine the MMTk-level ObjectReference which the JikesRVM binding gives to mmtk-core. The address of the JavaHeader of each object is a good candidate for the new definition because it is guaranteed to be inside an object. But from the software engineering's point of view,

  • we can't change the JikesRVM-level ObjectReference because it is used throughout the VM, and
  • we don't want to change the constants in mmtk-jikesrvm/mmtk/src/java_header_constants.rs to be relative to JavaHeader because it makes the code inconsistent and error-prone.

To solve this problem, we need to make a clear distinction between the MMTk-level ObjectReference and the JikesRVM-level ObjectReference. We'll create a new Rust type struct JikesObj(Address) which is equal to the JikesRVM-level ObjectReference. We can convert between JikesObj and (the MMTk-level) ObjectReference by adding/subtracting JAVA_HEADER_BYTES. All the header / status / length fields are accessed using JikesObj using existing constants which are relative to JikesObj.

The refactoring will take three steps. The first two steps are local to mmtk-jikesrvm. Only the last step involves changes to mmtk-core.

  • Step 1: Introducing the JikesRVM-specific JikesObj type. All methods that access object metadata and type info are off-loaded to the JikesObj type and other wrapper types such as TIB and RVMType. That leaves the implementation of the ObjectModel trait as simple as converting the MMTk-level ObjectReference parameter to JikesObj, and calling its methods. In this step, JikeObj and ObjectReference still have the same underlying usize value. This step only makes sure the new abstraction works.
  • Step 2: Redefining the MMTk-level ObjectReference to point to JavaHeader. If the abstraction from Step 1 is correct, we only need to rewrite the conversion function between ObjectReference and JikesObj, and change some constants (most importantly IN_OBJECT_ADDRESS_OFFSET) in VMObjectModel. This step makes sure the JikesRVM binding can work with a changed definition of MMTk-level ObjectReference.
  • Step 3: Modifying mmtk-core to require ObjectReference to point into an object. This will formally introduce the requirement into mmtk-core, and refactor functions (such as the algorithm that searches for the last VO bits) to take advantage of the new requirement. Since the JikesRVM binding already works with ObjectReference pointing into object bodies in Step 2, Step 3 should be as simple as removing redundant methods and constants in the ObjectModel trait.
wks added a commit that referenced this issue Sep 3, 2024
The main purpose of this PR is make a clear distinction between the
`ObjectReference` type in JikesRVM and the `ObjectReference` type in
mmtk-core.

This PR introduced `JikesObj`, a Rust type that represents the
JikesRVM-level `ObjectReference`. It needs an explicit conversion to
convert to/from the MMTk-level `ObjectReference` types.

The interface between mmtk-core and the mmtk-jikesrvm binding is
refactored to do fewer things with the MMTk-level `ObjectReference`.

- Trait methods that pass `ObjectReference` to the binding, notably the
methods in `ObjectModel`, now simply convert the MMTk-level
`ObjectReference` to `JikesObj`, and then call methods of `JikesObj`.
- Concrete methods for accessing object headers, fields, and layout
information are now implemented by `JikesObj` (and other wrapper types
including `TIB` and `RVMType`).
- The `JikesRVMSlot` trait now does the conversion between `JikesObj`
and the MMTk-level `ObjectReference` when loading or storing a slot.

This allows us to change the definition of the MMTk-level
`ObjectReference` in the future, while concrete methods of `JikesObj`
still use offset constants relative to the JikesRVM-level
`ObjectReference` which will not change.

The interface between the Rust part and the Java part of the binding are
refactored to involve `JikesObj` only.

- API functions in `api.rs` accept `JikesObj` parameters from JikesRVM
and return `JikeObj` to JikesRVM where JikesRVM uses the JikesRVM-level
`ObjectReference`.
- We wrap all JTOC calls into strongly-typed Rust functions, and make
the weakly-typed `jtoc_call!` macro private to the wrappers.

In this way, we ensure none of the API functions or JTOC calls leak the
MMTk-level `ObjectReference` values to JikesRVM, or accidentally
interpret a JikesRVM-level `ObjectReference` as an MMTk-level
`ObjectReference`.

We also do some obvious refactoring that makes the code more readable.:

- Encapsulated many field-loading statements in the form of `(addr +
XXXX_OFFSET)::load<T>()` into dedicated methods.
- Encapsulated the code for determining the overhead of hash fields into
a function `JikesObj::hashcode_overhead` and simplified many methods
that depend on that.
-   Renaming "edge" to "slot" in `RustScanThread.java`.

And obvious bug fixes:

- The call to `DO_REFERENCE_PROCESSING_HELPER_SCAN_METHOD_OFFSET` used
to erroneously interpret 0 as `true`. This has been fixed by relying on
the conversion trait.
- `scan_boot_image_sanity` used to declare an immutable array and let
unsafe `jtoc_call!` code modify it. The array is now defined as mutable.

Related issues and PRs:

- This PR is the 1st step of
#178
- It will ultimately allow mmtk/mmtk-core#1170
to be implemented.
wks added a commit that referenced this issue Sep 3, 2024
This PR changes the definition of MMTk-level `ObjectReference` for the
JikesRVM binding so that it now points to the JavaHeader, and is
different from the JikesRVM-level `ObjectReference` (a.k.a. `JikesObj`).
This will guarantee that the MMTk-level ObjectReference is always inside
an object.

Note that this PR does not involve a change in mmtk-core. It changes
`ObjectModel::IN_OBJECT_ADDRESS_OFFSET` to 0 so that the "in-object
address" is identical to the raw address of `ObjectReference`. It
demonstrates the JikesRVM binding can work well with MMTk-level
`ObjectReference` being different from JikesRVM-level `ObjectReference`.

Related issues and PRs.
-   This PR is based on #177
- This PR is the 2nd step of
#178
- It will ultimately allow mmtk/mmtk-core#1170
to be implemented.
wks added a commit to wks/mmtk-jikesrvm that referenced this issue Sep 5, 2024
This makes changes for upstream API changes.

Upstream PR: mmtk/mmtk-core#1195

Fixes: mmtk#178
mmtkgc-bot added a commit that referenced this issue Sep 6, 2024
This makes changes for upstream API changes.

Upstream PR: mmtk/mmtk-core#1195

Fixes: #178

---------

Co-authored-by: mmtkgc-bot <[email protected]>
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 a pull request may close this issue.

1 participant