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

Remove cast ref to mut everywhere #893

Merged
merged 12 commits into from
Aug 30, 2023
Merged

Remove cast ref to mut everywhere #893

merged 12 commits into from
Aug 30, 2023

Conversation

playXE
Copy link
Contributor

@playXE playXE commented Aug 8, 2023

  • MMTK type now stores Plan inside UnsafeCell to allow safe mutable access when necessary
  • Various types that previously had mut_self() method now are separated into Type and TypeInner, second one is stored inside Type behind UnsafeCell which allows to obtain mutable access when necessary without involving UB.
  • &'static references that were previously stored in some types are now converted into NonNull<>

@playXE
Copy link
Contributor Author

playXE commented Aug 8, 2023

Note: it still requires clippy::mut_from_ref for using &mut *UnsafeCell::get(), I am not entirely sure how to transition everything to safe code there.

@playXE playXE mentioned this pull request Aug 8, 2023
@playXE playXE requested a review from qinsoon August 10, 2023 08:54
@qinsoon
Copy link
Member

qinsoon commented Aug 10, 2023

Thanks for the PR. I will look at it tomorrow.

@qinsoon
Copy link
Member

qinsoon commented Aug 11, 2023

Again thanks for the PR. This is a good cleanup for identifying and removing cast_ref_to_mut for the issue #850.

As a summary, the changes in this PR are:

  • MMTK.plan uses UnsafeCell.
  • Prepare/Release.plan uses *const C::PlanType instead of &'static C::PlanType so we can cast it to a mutable pointer.
  • Add an inner type for a few types and use UnsafeCell<XInner> to allow mutability
    • FreeListPageResource
    • FragmentedMmapper
    • Map32
    • Map64
  • Add Send/Sync to types that uses unsafe pointers.
  • Use NonNull instead of &:
    • &'static CommonFreeListPageResource in Map32
    • &'static IntArrayFreeList in IntArrayFreeList

Generally, undefined behavior < unsafe code < safe code. However, this PR adds a large number of unsafe code (see the following table) to remove the undefined behavior code (which is also counted as unsafe in the table). As reducing unsafe code is one of our goals for the project (#97), this makes it hard to argue that this PR is an improvement for the code base.

unsafe master PR
functions 13 13
exprs 1414 1671
item_impls 14 28
item_traits 0 0
methods 40 50

(The data is from count-unsafe)

The urgency for this PR is that from Rust 1.72, cast_ref_to_mut (which is removed by the PR) will be a hard error. That means without any change, our code base won't build with Rust 1.72 (which will be released on 24th August). 24th August falls in our next release cycle, and we won't increase our pinned Rust version before next release. This means we have at least 6 weeks to address this issue.

I think the PR is good on its own, no change is needed. But it is a temporary solution for cast_ref_to_mut for Rust 1.72. Our options for the PR are:

  1. We merge this PR, but we should replace the unsafe code around those places with safe code as much as we can before the next release.
  2. We put this PR on hold, refactor related code in master to safe code as much as we can before the next release, and merge this PR with whatever is left.

My current preference is 2, considering that we will still have some time before Rust 1.72 and the next MMTk release.

@wks
Copy link
Collaborator

wks commented Aug 11, 2023

I am OK with either merging this PR in this release or the next release. I am OK with doing some refactoring before merging this PR, too, because that will mean this PR will need to change fewer places.

But as a reminder, some of the unsafe code introduced in this PR can be very hard to refactor fully. For example, fully fixing #852 requires refactoring the work packet scheduler, which should be part of the research topic of a PhD. However, a shallow refactoring may be possible. We can use Atomic and UnsafeCell to push the unsafe to specific fields.

Others should be easy to fix. #853 I think the FreeList data structure is low-level, and it can legitimately use unsafe.

@qinsoon
Copy link
Member

qinsoon commented Aug 11, 2023

But as a reminder, some of the unsafe code introduced in this PR can be very hard to refactor fully. For example, fully fixing #852 requires refactoring the work packet scheduler, which should be part of the research topic of a PhD. However, a shallow refactoring may be possible. We can use Atomic and UnsafeCell to push the unsafe to specific fields.

We do not plan to change the scheduler (the design or the algorithm) for now (which is a research goal for @wenyuzhao), but we intend to allow mutable access to Plan in work packets (safe access if possible) which is a pure engineering goal.

Others should be easy to fix. #853 I think the FreeList data structure is low-level, and it can legitimately use unsafe.

Not necessarily so. The reason for the issue is that most of the related code is a direct port of Java MMTk which has no concept about mutable vs immutable. The unsafe code was just used to work around the issues Rust raised. What we want is to figure out mutability, and allow Rust to reason and check for us.

Another example is RawMemoryFreeList. It is necessary as JikesRVM is metacircular and it needs an implementation of free list without doing any runtime allocation. We do not have those restrictions now. I feel we may not even need RawMemoryFreeList.

@qinsoon
Copy link
Member

qinsoon commented Aug 29, 2023

This is the performance for the PR (with OpenJDK/Immix). It does not introduce any measurable overhead.

PR893-20230829-142547

@qinsoon qinsoon added PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) and removed PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) labels Aug 29, 2023
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.

LGTM

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, too.

@qinsoon qinsoon enabled auto-merge August 30, 2023 07:44
@qinsoon qinsoon added this pull request to the merge queue Aug 30, 2023
Merged via the queue into mmtk:master with commit 66e8cb8 Aug 30, 2023
15 of 19 checks passed
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.

4 participants