Move prepare/release/end_of_gc to trait Space #1315
Conversation
There was a problem hiding this comment.
After a second thought, I think it is not a good idea to "unify" the prepare, release and end_of_gc methods into the Space trait.
The obvious problem is the arg argument in prepare. Actually each space expects a specific arg type. Specifically, CopySpace expects a bool, while ImmixSpace expects a StatsForDefrag. Making it a Option<Box<dyn std::any::Any>> simply makes it dynamically types, and errors when passing the argument cannot be detected until run time. On the contrary, when using existing way, the Plan knows the concrete types of its spaces, and calls the prepare methods of each space differently, providing exactly what type of argument required. And that's type-checked by the Rust compiler.
The purpose of this PR is to support the polymorphic non-moving space (#1308). The prepare method is still called differently depending on the concrete space type.
In my refactoring I made a trait NonMovingSpace and its prepare_nonmoving_space takes only a full_heap parameter (passed to major_gc in prepare of some spaces). I admit that it is not general, either. It only works coincidentally because full_heap happens to be the union of all arguments of ImmortalSpace, MarkSweepSpace and ImmixSpace.
|
|
||
| // Prepare defrag info | ||
| if self.is_defrag_enabled() { | ||
| let plan_stats = arg.unwrap().downcast::<StatsForDefrag>().unwrap(); |
There was a problem hiding this comment.
This means ImmixSpace is expecting a StatesForDefrag argument.
| @@ -138,6 +139,39 @@ impl<VM: VMBinding> Space<VM> for CopySpace<VM> { | |||
| fn enumerate_objects(&self, enumerator: &mut dyn ObjectEnumerator) { | |||
| object_enum::enumerate_blocks_from_monotonic_page_resource(enumerator, &self.pr); | |||
| } | |||
|
|
|||
| fn prepare(&mut self, _major_gc: bool, arg: Option<Box<dyn Any>>) { | |||
| let is_from_space = arg.unwrap().downcast::<bool>().unwrap(); | |||
There was a problem hiding this comment.
This means CopySpace expects a bool
I am okay with closing this PR. That means we understand that most spaces have |
|
See the discussion in this PR and in #1316. We do not want to do this change for now. |
No description provided.