Skip to content

Remove ProcessEdgesBase::worker #597

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
# for free to join this conversation on GitHub. Already have an account? # 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