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

Rename associated types in GCWorkContext #1116

Merged
merged 4 commits into from
Apr 16, 2024
Merged

Conversation

wks
Copy link
Collaborator

@wks wks commented Apr 15, 2024

This PR renames the associated types ProcessEdgesWorkType to DefaultProcessEdges, and TPProcessEdges to PinningProcessEdges.

The old name TPProcessEdges was very confusing because that type alone is not sufficient to implement transitive pinning roots. We rename it to PinningProcessEdges, removing "transitive" from its name. We also renamed ProcessEdgesWorkType to DefaultProcessEdges for symmetry. We also updated comments to clarify the purposes of those type members.

We also updated the type parameters of ProcessEdgesWorkRootsWorkFactory<VM, DPE, PPE> and ProcessRootNode<VM, R2OPE, O2OPE> to clarify their purposes, at the expense of being slightly more verbose.

wks added 2 commits April 15, 2024 17:02
This PR renames `ProcessEdgesWorkType` to `NormalProcessEdges`, and
renames `TPProcessEdges` to `PinningProcessEdges`.

The old name `TPProcessEdges` was very confusing because the type alone
is not sufficient to implement transitive pinning roots.  We rename it
to PinningProcessEdges, removing "transitive" from its name.  We also
renamed `ProcessEdgesWorkType` to `NormalProcessEdges` for symmetry.

FYI, non-transitive and transitive pinning roots are implemented using a
combination of `NormalProcessEdges` and `PinningProcessEdges`.

-   We use `PinningProcessEdges` for objects directly pointed from
    non-transitive pinning roots, but `NormalProcessEdges` for their
    descendents.
-   We use `PinningProcessEdges` to trace all objects reachable from
    transitive pinning roots.
@wks wks requested a review from qinsoon April 16, 2024 04:07
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

@k-sareen
Copy link
Collaborator

Normal -> Default perhaps?

@wks
Copy link
Collaborator Author

wks commented Apr 16, 2024

I did the obvious change -- renaming those type members and some generic type arguments.

I considered making is_pinning: bool a parameter of trace_object. (See: https://mmtk.zulipchat.com/#narrow/stream/262673-mmtk-core/topic/Pinning.20and.20TPProcessEdges/near/432461772) It may need some non-trivial refactoring, and may potentially change the API (I can already anticipate changing the public method ObjectTracer::trace_object). I'll try to do the refactoring the next time.

@wks
Copy link
Collaborator Author

wks commented Apr 16, 2024

Normal -> Default perhaps?

Yes. That sounds better.

@wks wks added this pull request to the merge queue Apr 16, 2024
Merged via the queue into mmtk:master with commit a2b36f4 Apr 16, 2024
23 checks passed
@wks wks deleted the rename-tpprocessedges branch April 16, 2024 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants