Skip to content

Add ExternalPageResource and allow discontiguous VM space#864

Merged
qinsoon merged 13 commits intommtk:masterfrom
qinsoon:discontiguous-vm-space
Aug 31, 2023
Merged

Add ExternalPageResource and allow discontiguous VM space#864
qinsoon merged 13 commits intommtk:masterfrom
qinsoon:discontiguous-vm-space

Conversation

@qinsoon
Copy link
Copy Markdown
Member

@qinsoon qinsoon commented Jul 4, 2023

This PR changes VMSpace to multiple discontiguous regions as VM space. This PR is used in mmtk/mmtk-julia#71 to support system and package images.

  • add an ExternalPageResource to manage the discontiguous regions.
  • implement VMSpace with ExternalPageResource. VMSpace no longer uses ImmortalSpace.
  • rename lazy_init_vm_space to set_vm_space, as we allow setting vm space multiple times with non overlapping regions.

@qinsoon
Copy link
Copy Markdown
Member Author

qinsoon commented Jul 4, 2023

Currently we cannot run with MSRV 1.61 at the moment. We have a dependency chain mmtk->env_logger->is_terminal, is_terminal updated to MSRV 1.63 in 0.4.8 and env_logger uses is_terminal 0.4.0 (including any point versions). We can wait until they resolve the issue. Otherwise, we can bump our MSRV. rust-cli/env_logger#273

@qinsoon
Copy link
Copy Markdown
Member Author

qinsoon commented Jul 4, 2023

From rust-cli/env_logger#273, the options for us are:

  1. Bump our MSRV to 1.63 -- this will resolve the issue for now, but we may see the same problem in the future until cargo/Rust improves on version resolution for MSRV.
  2. Check in our Cargo.lock and specify a version that is compatible with MSRV 1.61. This is not recommended by the current Rust documentation, but is in use by some Rust official projects (like env_logger) and there is an issue that suggest to make it recommended to check in lock file for libraries Check Cargo.lock in version control for libraries rust-lang/cargo#8728.

I would suggest 2. It does not look like cargo/Rust will improve on this any time soon, and 2 does no harm.

@wks
Copy link
Copy Markdown
Collaborator

wks commented Jul 4, 2023

We can also play our old trick of locking a dependency to a specific version until the problem is solved. We once did this for enum_map.

diff --git a/Cargo.toml b/Cargo.toml
index db4da419d..6f07eb851 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -28,6 +28,7 @@ delegate = "0.9.0"
 downcast-rs = "1.1.1"
 enum-map = "2.4.2"
 env_logger = "0.10.0"
+is-terminal = "=0.4.7"  # latest version requires MSRV 1.63
 itertools = "0.10.5"
 jemalloc-sys = { version = "0.5.3", features = ["disable_initial_exec_tls"], optional = true }
 lazy_static = "1.1"

@qinsoon qinsoon force-pushed the discontiguous-vm-space branch from 8e06b28 to 7d19f58 Compare July 4, 2023 05:26
@qinsoon qinsoon force-pushed the discontiguous-vm-space branch from 7d19f58 to 7fc052e Compare July 4, 2023 05:33
@qinsoon qinsoon marked this pull request as ready for review August 15, 2023 03:07
@qinsoon qinsoon requested a review from wks August 15, 2023 03:07
Co-authored-by: Kunal Sareen <[email protected]>
@qinsoon
Copy link
Copy Markdown
Member Author

qinsoon commented Aug 28, 2023

binding-refs
JULIA_BINDING_REPO=qinsoon/mmtk-julia
JULIA_BINDING_REF=update-julia-upstream-43bf2c8

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Aug 28, 2023
@qinsoon
Copy link
Copy Markdown
Member Author

qinsoon commented Aug 29, 2023

This PR is ready for review again. @wks

Copy link
Copy Markdown
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 added this pull request to the merge queue Aug 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 30, 2023
@qinsoon qinsoon enabled auto-merge August 30, 2023 05:29
@qinsoon qinsoon disabled auto-merge August 30, 2023 05:30
@qinsoon qinsoon enabled auto-merge August 30, 2023 06:30
@qinsoon qinsoon removed the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Aug 30, 2023
@qinsoon qinsoon added this pull request to the merge queue Aug 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 30, 2023
@qinsoon qinsoon enabled auto-merge August 31, 2023 00:19
@qinsoon qinsoon added this pull request to the merge queue Aug 31, 2023
Merged via the queue into mmtk:master with commit a504184 Aug 31, 2023
@qinsoon qinsoon deleted the discontiguous-vm-space branch August 31, 2023 01:48
github-merge-queue bot pushed a commit that referenced this pull request Aug 7, 2025
We once used `ImmortalSpace` for VM space.
#864 introduced a separate policy
for `VMSpace`, and these code in `ImmortalSpace` became dead code since
then. This PR removes the dead code.

---------

Co-authored-by: Kunal Sareen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants