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

Refactor scheduler and worker creation #539

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Conversation

wks
Copy link
Collaborator

@wks wks commented Feb 4, 2022

This is part 2 of refactoring the scheduler: #532

But don't merge it yet, because there is a known problem: it breaks options from command line. See below. For now we temporarily disable setting the number of GC threads from the command line.

We now construct the GCWorkScheduler in one step, so that we no longer need to make every field Option<T> and set the value during initialize.

To make it possible, we eliminated cyclic references by considering MMTK, GCWorkScheduler and GCWorkerShared as nested data, like this:

MMTK {
  ...
  sheduler: GCWorkScheduler {
    ...
    workers: vec![
      GCWorkerShared {...},
      GCWorkerShared {...},
      GCWorkerShared {...},
      ...
    ],
  }
}

So inner structures do not refer to outer structures. We eliminated the GCWorkScheduler::mmtk field.

On the other hand, actors (threads) have references to all shared data they need. Both GCWorker and GCController contain a reference to the MMTK instance, so they no longer need to navigate from the scheduler to MMTK. And, obviously, private data of the actors (threads, including GCWorker and GCController) never refer to one another.

Known problem

We create GCWorkScheduler when MMTK is created. This is too early for now, because the MMTK instance is usually created statically by the VM. When it is created, we still don't know how many threads we have. The number of threads is determined by the mmtk.options.threads option, which can be set by either environment variable or command-line options. This is too late for the creation of GCWorkScheduler. The current PR will simply disregard any changes to the value of mmtk.options.threads after GCWorkScheduler is created.

To solve this problem, we need to make one (or both) of the following changes before applying this PR.

  1. Introducing a MMTKBuilder, and build the MMTK instance after options are parsed.
  2. Make it possible to add workers after GCWorkScheduler is created.
  3. Change MMTK::scheduler to Option<Arc<GCWorkScheduler>>.

We will need to do (1) anyway, because currently options are also held by an UnsafeOptionsWrapper, which is unsafe.

Method (2) needs to make the worker group synchronised (probably using a RwLock). Adding a RwLock directly may add performance overhead because the work packet system frequently queries if all workers are parked. To do it efficiently, we should redesign the synchronisation algorithm and make the lock more coarse-grained. Issue #537 covers some of the details.

Method (2) will also enable the potential of dynamically adjusting the number of workers during execution, but let's not attempt that for now.

Method (3) simply pushes the Option<T> from the fields of GCWorkScheduler to the field of MMTK. It simply moves the problem to a different place.

@wks wks force-pushed the refactor-scheduler2 branch 3 times, most recently from 66cb6d4 to 91f54e3 Compare February 7, 2022 05:41
@wks wks marked this pull request as ready for review February 7, 2022 05:47
@wks wks requested a review from qinsoon February 7, 2022 07:53
@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Feb 8, 2022
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

This commit eliminates unnecessary Option<T> fields, two-phase
initialization and unsafe get_mut_unchecked call in GCWorkScheduler.

We consider MMTK, GCWorkScheduler and GCWorkerShared as shared data
between threads, and GCWorker and GCController as private data of the
workers and the controller.  We now create shared data, including all
GCWorkerShared structs, before spawning any GC threads.  This means we
no longer spawn GC threads before GCWorkScheduler is fully initialized,
and eliminated some unsafe operations.

We temporarily make Options::threads only settable via environment
variable, because we now create GCWorkerShared instances when
GCWorkScheduler is created, which is usually static.
@qinsoon qinsoon merged commit 307c63a into mmtk:master Feb 11, 2022
@wks wks mentioned this pull request Jul 19, 2022
5 tasks
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.

2 participants