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 ExternalPageResource and allow discontiguous VM space #864

Merged
merged 13 commits into from
Aug 31, 2023

Conversation

qinsoon
Copy link
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
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
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
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 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
src/policy/vmspace.rs Outdated Show resolved Hide resolved
src/policy/vmspace.rs Show resolved Hide resolved
src/policy/vmspace.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
Co-authored-by: Kunal Sareen <[email protected]>
@qinsoon
Copy link
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
Member Author

qinsoon commented Aug 29, 2023

This PR is ready for review again. @wks

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 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
16 of 19 checks passed
@qinsoon qinsoon deleted the discontiguous-vm-space branch August 31, 2023 01:48
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