Rename edge to slot #1134
Rename edge to slot #1134
Conversation
Parent PR: mmtk/mmtk-core#1134
Parent PR: mmtk/mmtk-core#1134
Parent PR: mmtk/mmtk-core#1134
Parent PR: mmtk/mmtk-core#1134
Parent PR: mmtk/mmtk-core#1134
|
binding-refs |
|
The CI test |
|
This PR is now ready for review. Binding PRs: |
There was a problem hiding this comment.
We would like to rename trait Edge to trait Slot, as trait Edge was a misuse, and it always means slots. But we can still use the term 'edge' in many other places, when we actually mean edges.
For example, things like edges: Vec<Slot> make sense. edges: Vec<ObjectReference> also makes sense. But I am okay with slots: Vec<Slot>.
However, there are a few cases I think we went too far for renaming, e.g. having ProcessEdgesWork::process_slot rather than ProcessEdgesWork::process_edge, creating the term 'slot root' in create_process_slot_root_work. Based on the premise that the goal of this PR is to make the code easier to understand, my suggestion is that we do not do renaming unless the current code/comment is actually wrong/confusing. We can discuss case by case.
src/vm/scanning.rs
Outdated
| /// * `edges`: A vector of edges. | ||
| fn create_process_edge_roots_work(&mut self, edges: Vec<ES>); | ||
| /// * `slots`: A vector of slots. | ||
| fn create_process_slot_roots_work(&mut self, slots: Vec<SL>); |
There was a problem hiding this comment.
I don't think we want to create a term 'slot root'.
We can just call this create_process_roots_work. create_process_roots_work vs create_process_(t)pinning_roots_work makes sense.
If you really want to emphasise that it is processing slots, you can say create_process_root_slots_work. But I feel it is unnecessary.
There was a problem hiding this comment.
That makes sense. "slot roots" is not the opposite of "pinning roots".
- The opposite of "slot roots" (or root slots) is "node roots" (or root nodes), and we already have
ProcessRootNodes. - The opposite of "pinning roots" is "non-pinning roots".
There is one missing piece of the puzzle: Root nodes that can actually be moved. The current RootsWorkFactory does not allow that, but I already anticipated that (See #710), and the Android binding (which @k-sareen is developing) needs that to scan and update stack roots and global roots. But the problem is, such roots cannot be given as a list of ObjectReference or a list of Slot (as ObjectReference alone cannot update the stack slots), so it will probably not be called create_process_root_nodes_work.
But I think we should keep the word "slots" in the function name so that we emphasize that it is just one way to deliver roots to mmtk-core, and it is a list of slots, and we can distinguish it from other functions (existing or introduced in the future) that does not represent roots as Slot. create_process_root_slots_work sounds good to me. I'll use that name in this PR. In the future, however, we may refactor the RootsWorkFactory again to address #710.
There was a problem hiding this comment.
The other functions like create_process_(t)pinning_roots_work focus on the semantics of pinning/tpining, rather than how the roots are passed as arguments. It still feels strange as create_process_root_slots_work cares about the arguments rather than the semantics.
There was a problem hiding this comment.
It's a bit subtle. The fact that a root is represented as Slot implies it can be updated (using slot.store. i.e. It's not pinned), and the fact that a root is represented as an ObjectReference implies it cannot be updated (i.e. pinned). create_process_roots_work is just too general because the word "roots" covers both pinning roots and non-pinning roots.
One way to reconcile is renaming the functions for nodes, too:
| old | new |
|---|---|
create_process_edge_roots_work |
root_slots |
create_process_pinning_roots_work |
pinning_root_nodes |
create_process_tpinning_roots_work |
tpinning_root_nodes |
(Lets' just omit the repeating create_process_ and _work.)
In this way, we mention both the form (slots / nodes) and the semantics (unrestricted / pinning / tpinning). What do you think about this?
There was a problem hiding this comment.
We should minimise changes to the existing interface. The only thing that needs to be fixed here is 'edge roots', which is a clear misnomer.
If we introduced the API in this PR, what you proposed would be okay. But for existing APIs, the changes seem unnecessary.
There was a problem hiding this comment.
Then I think we can just go with create_process_slot_roots_work this time.
'slot root' is a new misnomer as well. If we change from one misnomer to another misnomer, we could just not do the change at all. In that case, at least we minimise changes to the existing interface.
There was a problem hiding this comment.
Then I'll change it to create_process_root_slots_work.
There was a problem hiding this comment.
Then it is back to this question.
The other functions like create_process_(t)pinning_roots_work focus on the semantics of pinning/tpining, rather than how the roots are passed as arguments. It still feels strange as create_process_root_slots_work cares about the arguments rather than the semantics.
There was a problem hiding this comment.
Then we have two solutions. One is renaming those functions, too, as I described, which resulted in the table in this comment. The other is leaving them unchanged for now. That's not ideal, but we minimise the change, and don't introduce new misnomers.
There was a problem hiding this comment.
I am okay with the latter -- just leave it unchanged before we can have a good fix for it.
Stop using "process_edges" to refer to the work packet. The work packet name is ProcessEdgesWork, and the method name is process_slots.
|
I have approved this PR and related binding PRs. We will need a section for this PR in the new migration guide. Depending on the merging order, we can add the section in this PR or in #1133. |
Parent PR: mmtk/mmtk-core#1134 --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>
Parent PR: mmtk/mmtk-core#1134 --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>
Parent PR: mmtk/mmtk-core#1134 --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>
Parent PR: mmtk/mmtk-core#1134 --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>
Parent PR: mmtk/mmtk-core#1134 --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>
Parent PR: mmtk/mmtk-core#1134 --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com> (cherry picked from commit e776c41) # Conflicts: # mmtk/Cargo.lock # mmtk/Cargo.toml # mmtk/src/api.rs # mmtk/src/julia_scanning.rs # mmtk/src/lib.rs # mmtk/src/scanning.rs # mmtk/src/slots.rs
Parent PR: mmtk/mmtk-core#1134 --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>
This commit changes the term "edge" to "slot" where it actually refers to a field of an object or a variable on the stack or in any global data structures. Notably, the trait
Edgeis renamed toSlot, and related types and methods are renamed, too. We still use the term "edge" to refer to edges in the object graph, regardless whether the edge is represented by a slot or by an object reference pointing to the target.The work packet trait
ProcessEdgesWorkand its implementations still use the term "edge". Although it was originally designed to process slots, it can be used (and is currently actually used) in node-enqueuing tracing as well as weak reference processing, in which case it is used as a provider of thetrace_objectmethod which traces object graph edges represented asObjectReferenceto the target objects.Note: This commit only does renaming, and does not change the program logic. The behavior of the program should be exactly the same after this change.
Fixes: #687