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

Add back DummyVM as a part of the porting guide. Minor changes to MMTk initialization in the porting guide. #1142

Merged
merged 19 commits into from
Jun 5, 2024

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Jun 3, 2024

Following the discussion here: https://mmtk.zulipchat.com/#narrow/stream/315620-Porting/topic/Porting.20MMTK.20to.20Clasp.20Common.20Lisp/near/442123897. It is useful for the language implementers to have a Rust binding crate that implements all the boilerplate code and can compile to start with. This PR adds back DummyVM to the porting guide, and includes some minor changes to the porting guide.

@qinsoon qinsoon requested a review from wks June 3, 2024 05:13
@wks
Copy link
Collaborator

wks commented Jun 3, 2024

We shouldn't bury the dummyvm directory too deep into the docs directory. MMTk core developers will need to keep maintaining it as we change the API of mmtk-core. It is better to move it to a more accessible location. I suggest we place it in mmtk-core/dummyvm, directly. We may add a README.md in mmtk-core/dummyvm to state its purpose and link to the porting guide.

@qinsoon
Copy link
Member Author

qinsoon commented Jun 3, 2024

We shouldn't bury the dummyvm directory too deep into the docs directory. MMTk core developers will need to keep maintaining it as we change the API of mmtk-core. It is better to move it to a more accessible location. I suggest we place it in mmtk-core/dummyvm, directly. We may add a README.md in mmtk-core/dummyvm to state its purpose and link to the porting guide.

I put it as a part of the porting guide, as I feel it is only relevant for people who start a new binding. I don't think it is important and relevant enough to be at the top level of the project root directory. It was placed in vmbindings/dummyvm, because it was introduced when we had other bindings in the core repo such as vmbindings/jikesrvm and vmbindings/openjdk.

For maintainability, I modified the CI scripts to build and style-check DummyVM in the CI (in the same way as we check the mygc in the tutorial). We will know if it no longer compiles. Also I changed the DummyVM a bit to have more methods as unimplemented!(), as we no longer need it to run with the default implementation. We just need it to compile so people can have a reasonable start point for porting.

If you do think we should put it as a place that is easier for people to browse to from the root directory, I think we can put it in mmtk-core/docs/dummyvm.

@wks
Copy link
Collaborator

wks commented Jun 3, 2024

If you do think we should put it as a place that is easier for people to browse to from the root directory, I think we can put it in mmtk-core/docs/dummyvm.

Yes. mmtk-core/docs/dummyvm is good enough, at least shorter than the previous mmtk-core/vmbindings/dummyvm. We can add a sentence in its own README.md which says it is probably only relevant for people who start a new binding.

And we can remove mmtk-core/docs/header given that we acknowledge that dummyvm is part of the documentation. There is another mmtk.h in docs/header, and we can remove that in favor for the one in dummyvm.

docs/dummyvm/Cargo.toml Outdated Show resolved Hide resolved
@qinsoon
Copy link
Member Author

qinsoon commented Jun 4, 2024

The 3 optional checks failed, but they are expected:

  • The broken link check failed as I added a link to the dummyvm directory in the github repo, which does not exist yet.
  • The API check failed, as I exposed InitializeOnce from mmtk-core so it can be used for SINGLETON.
  • The above API change does not require an entry in the migration check. The migration guide check failed, as we have an API change but no migration guide change.

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

There are still some occurrences of "edge" in "slots.rs". They need to be renamed.

But in practice, no VM uses so many kinds of slots. I introduced those slots into the old DummyVM in order to test if all those kinds of slots work, and now we have test cases in mock_test_slots.rs. To make a template for new bindings, we may just set DummyVM::SlotType to mmtk::vm::slot::SimpleSlot and remove this slots.rs. Alteranatively, we may implement a DummyVMSlot by copying from SimpleSlot. That should be enough for documentation and demonstration.

docs/dummyvm/src/scanning.rs Outdated Show resolved Hide resolved
docs/dummyvm/src/slots.rs Outdated Show resolved Hide resolved
docs/dummyvm/src/slots.rs Outdated Show resolved Hide resolved
docs/dummyvm/src/slots.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

The methods ref_to_object_start, ref_to_header, ref_to_address and address_to_ref were previously defined for the convenience of some test cases, too. We can change them to be more instructive.

docs/dummyvm/src/object_model.rs Outdated Show resolved Hide resolved
docs/dummyvm/src/object_model.rs Outdated Show resolved Hide resolved
docs/dummyvm/src/object_model.rs Show resolved Hide resolved
docs/dummyvm/src/slots.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM

@qinsoon qinsoon enabled auto-merge June 5, 2024 05:09
@qinsoon qinsoon added this pull request to the merge queue Jun 5, 2024
Merged via the queue into mmtk:master with commit 401803c Jun 5, 2024
23 of 24 checks passed
@qinsoon qinsoon deleted the portingguide-init-details branch June 5, 2024 06:44
wks added a commit to wks/mmtk-core that referenced this pull request Jun 11, 2024
`docs/header/mmtk.h` was removed in
mmtk#1142 in favor for the header in
the DummyVM, but the link was not updated.
@wks wks mentioned this pull request Jun 11, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jun 11, 2024
`docs/header/mmtk.h` was removed in
#1142 in favor for the header in
the DummyVM, but the link was not updated.
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.

3 participants