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 ProcessEdgesBase::worker #597

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Remove ProcessEdgesBase::worker #597

wants to merge 2 commits into from

Conversation

wks
Copy link
Collaborator

@wks wks commented May 18, 2022

The ProcessEdgesBase::worker field is a *mut GCWorker<VM>. This is unsafe.

pub struct ProcessEdgesBase<VM: VMBinding> {
    pub edges: Vec<Address>,
    pub nodes: Vec<ObjectReference>,
    mmtk: &'static MMTK<VM>,
    worker: *mut GCWorker<VM>,  // This is unsafe. Remove it
    pub roots: bool,
} 

ProcessEdgesWork needs to access the worker instance because

  1. XxxxxxSpace::trace_object needs a GCWorker<VM> reference to get the CopyContext, and
  2. flush() needs a GCWorker<VM> to submit or execute ScanObjects work packets.

Currently, ProcessEdgesWork attempts to give itself access to GCWorker by holding a *mut GCWorker<VM> in the ProcessEdgesBase::worker field. This is unsafe, because the work packet is not associated to a GCWorker until it is executed by a GCWorker. The access to the *mut GCWorker<VM> pointer is invalid before do_work starts, and after do_work finishes.

In idiomatic Rust, if the GCWorker is only valid to access during the execution of gc_work, it should be passed to gc_work as a &mut GCWorker<VM> reference. Rust's borrowing semantics will ensure that it is only borrowed during the execution of gc_work.

Actually GCWork::do_work already has a worker: &mut GCWorker<VM> parameter. So we can pass it through levels of function calls to give them access to GCWorker.

This PR attempts to do this.

DRAFT: I added &mut GCWorker parameter to too many functions, but only a few functions actually use it. This may indicate that there is still some room for refactoring. One possibility is to introduce a struct that has a lifetime parameter 'w and contains a &'w mut GCWorker, and use that struct as the self, but only during the execution of do_work.

wks added 2 commits May 18, 2022 17:08
@wks wks added C-refactoring Category: Refactoring A-work-queue Area: Work queue G-safety Goal: Safety labels May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-work-queue Area: Work queue C-refactoring Category: Refactoring G-safety Goal: Safety
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant