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

Problems with the public API #658

Open
wks opened this issue Sep 1, 2022 · 2 comments
Open

Problems with the public API #658

wks opened this issue Sep 1, 2022 · 2 comments
Labels
P-normal Priority: Normal.

Comments

@wks
Copy link
Collaborator

wks commented Sep 1, 2022

The public API of the mmtk-core to the VM is defined as all public items in the mmtk-core crate. Now they are fully listed here: https://www.mmtk.io/mmtk-core/public-doc/plan/index.html

We can now observe problems by looking at that document.

memory_manager

It is a legacy from JikesRVM MMTk. It contains many static (Java) or top-level (Rust) functions. They used to be the set of functions MMTk provide to JikesRVM. The problems of the memory_manager module in Rust MMTk are:

  1. They are not idiomatic in Rust. A Rust crate exposes its interface through modules, structs, enums, traits, functions and constants. Although Rust is not as object-oriented as C++ and Java, it is still more natural for Rust to use abstract data types and expose methods around types.
  2. They are incomplete. Some VMs still need to interact with types and functions in other modules to use needed functionalities of MMTk.

I suggest make transition gradually

  1. Deprecate the functions in memory_manager
  2. Implement those functions in relevant parts of MMTk.
    • If a function is related to a MMTk instance, make it a method of MMTK.
      • Example: memory_manager::bind_mutator to MMTK::bind_mutator
    • Similarly, if it is related to another object, make it a method of that.
      • Example: memory_manager::alloc to Mutator::alloc
    • If it is a top-level function, make it a top-level function of a related module.
      • Example: memory_manager::malloc to util::malloc::malloc. Expose util::malloc as pub.
  3. Migrate bindings to remove deprecation warnings.
  4. Remove the memory_manager module.

Things that shouldn't have been exported

The following are about plans:

  • plan::Plan: Contains dyn methods of a plan. A VM shouldn't directly interact with plans.
  • plan::PlanConstraints: Constraints of a plan. VMs probably never access them directly, but the VM does need to filter plans based on some criteria, such as whether a plan can pin objects.

The following are for work packets and computing the transitive closure. The VM used to create work packets which involves some of tracing, but we refactored the API before, and the VMs never see work packets since then. Now they shouldn't be exposed.

Some VMs used to do some hacks to implement object-enqueuing tracing. That needed ProcessEdgesWork and work-packet-related APIs, too. But they are not needed now.

  • scheduler::ProcessEdgesWork: VMs used to create this work packet to scan VM-specific roots.
  • scheduler::ScanStackRoot: VMs used to create this work packet to scan stack roots.
  • scheduler::WorkBucketStage: When VMs create ScanStackRoot, they specify the work bucket stage.
  • memory_manager::add_work_packet: VMs used to add work packets using this API
  • memory_manager::add_work_packets: and this API.
  • scheduler::GCWork: Used by memory_manager::add_work_packet.

These may be accidentally exposed

  • plan::ObjectQueue: Formerly called TransitvieClosure, it is used by the ProcessEdgesWork work packet to collect nodes while tracing objects.
    • plan::VectorObjectQueue: An implementation of ObjectQueue using vector.
  • plan::ObjectsClosure: Used by the ScanObjects work packet to collect edges while scanning objects.
  • plan::PolicyCopyContext: Used for copying objects.
  • scheduler::CoordinatorWork: Work packets done by the coordinator. The only coordinator work now is StopMutators.

Utils

We need to look into the util module more carefully

@qinsoon
Copy link
Member

qinsoon commented Sep 1, 2022

Things that shouldn't have been exported

The following are about plans:

  • plan::Plan: Contains dyn methods of a plan. A VM shouldn't directly interact with plans.

ActivePlan::global() returns a Plan. I think Plan is okay to be public, but we should check the visibility of each of its methods.
We also should consider removing ActivePlan::global(), though it is ported from Java MMTk. Currently it is almost a hack for mmtk-core code to access the current plan without needing to store a reference to it. I have done some refactoring to remove its use before, but there are still some stubborn cases that I didn't manage to remove.

  • plan::PlanConstraints: Constraints of a plan. VMs probably never access them directly, but the VM does need to filter plans based on some criteria, such as whether a plan can pin objects.

VMs will access them, e.g. max non LOS object size, the barrier that a plan is using, etc. They need those info to implement fastpaths. We could set visibility separately for each field in PlanConstraints.

@wks wks mentioned this issue Nov 3, 2022
@qinsoon
Copy link
Member

qinsoon commented Nov 10, 2022

If we plan to remove memory_manager which exports the most common API functions that we expect a binding to use, we need to improve our documentation (e.g. the porting guide #621) to provide a list of API methods that are essential for bindings. Otherwise it would be hard for a binding implementer to figure out what API he needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-normal Priority: Normal.
Projects
None yet
Development

No branches or pull requests

3 participants