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

Send/Sync cleanup #268

Merged
merged 15 commits into from
Apr 7, 2021
Merged

Send/Sync cleanup #268

merged 15 commits into from
Apr 7, 2021

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Mar 21, 2021

This PR cleans up our code around the Send and Sync traits. It is one step for #97.

  • Remove wrong supertraits of Send/Sync for certain types. For example:
    • Plan is Sync as it is shared across thread, but it is not Send as we do not send its ownership to another thread.
    • Work is Send as it will be sent to worker threads, but it is not Sync as we should not share it across threads.
  • Moving to unsafe impl Send/Sync to the exact type where it is needed.
    • For example, GenCopy is Sync, but it uses CommonPlan, and CommonPlan uses UnsafeCell which prevents Rust deriving Sync for CommonPlan safely. We should unsafe impl Sync for CommonPlan rather than GenCopy.
  • Remove unnecessary unsafe impl Send/Sync.

This PR should only be merged after we release 0.3.

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Mar 21, 2021
@qinsoon qinsoon requested review from javadamiri and wenyuzhao March 22, 2021 05:03
@qinsoon qinsoon marked this pull request as ready for review March 22, 2021 05:03
Copy link
Member

@wenyuzhao wenyuzhao 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
Contributor

@javadamiri javadamiri 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 merged commit bb6c2f5 into master Apr 7, 2021
@qinsoon qinsoon deleted the send-sync-cleanup branch April 7, 2021 01:58
qinsoon added a commit that referenced this pull request Apr 19, 2021
#268) (#275)

* Remove the cyclic reference between Space and PR
* Move head_discontiguous_region to CommonPageResource. Refactor
PageResource.alloc_pages() to return a result, and grow_space() in
Space.
* Remove UnsafeCell for CommonSpace
* Move zeroing code to Space
k-sareen pushed a commit to k-sareen/mmtk-core that referenced this pull request Jun 10, 2021
* Remove unsafe impl Send/Sync for MMTK. Remove unsafe cast from
Arc<Scheduler> to &'static Scheduler
* Removed manual impl of Send/Sync for MutatorConfig, each plan, and
controll collector context.
* Remove unsafe impl of Sync/Send for a few work packets
* Refactor for Scheduler.channel
* Explain unsafe Sync for spaces. Remove some unsafe Sync/Send for SFT.
* Only need Send for Work/GCWork/ProcessEdges (not Sync)
* Remove unsafe impl for WorkBucket
* Remove unnecessary Send/Sync super trait for some traits.
* Some comments about unsafe Sync/Send
k-sareen pushed a commit to k-sareen/mmtk-core that referenced this pull request Jun 10, 2021
mmtk#268) (mmtk#275)

* Remove the cyclic reference between Space and PR
* Move head_discontiguous_region to CommonPageResource. Refactor
PageResource.alloc_pages() to return a result, and grow_space() in
Space.
* Remove UnsafeCell for CommonSpace
* Move zeroing code to Space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants