mmtk-core icon indicating copy to clipboard operation
mmtk-core copied to clipboard

Cleanup `PageResource` and `{FreeList,Monotone}PageResource`

Open k-sareen opened this issue 3 years ago • 1 comments

While I was debugging heap accounting for the generational GC performance issue, I found it really hard to read and understand the PageResource trait and its implementations: {FreeList,Monotone}PageResource. It uses a mix of javadoc and rustdoc comment styles, and there are almost no comments explaining how or what it's doing. Since this is a core part of our virtual memory management code, I think it and its semantics should be well documented.

I note that there is a similar issue that exists (https://github.com/mmtk/mmtk-core/issues/60), but it suggests removing MonotonePageResource for having a unified PageResource implementation based on FreeListPageResource. I don't know of the merits of such a refactor, since PageResource should be thought of an interface without knowing implementation specific details, in my opinion.

k-sareen avatar Jun 27 '22 07:06 k-sareen

Honestly, same is also true for the Map trait and its implementations (Map{32,64}). A lot of the virtual memory management code is very poorly documented.

k-sareen avatar Jun 27 '22 08:06 k-sareen

Those types originate from the MMTk in JikesRVM. The documentation in the original JikesRVM is good enough to explain the main ideas of those types, but may not accurately reflect what the code in the Rust MMTk (and the JikesRVM MMTk) is actually doing.

The following links may help us reading the code

  • GenericFreeList: https://github.com/JikesRVM/JikesRVM/blob/master/MMTk/src/org/mmtk/utility/GenericFreeList.java
  • FreeListPageResource: https://github.com/JikesRVM/JikesRVM/blob/master/MMTk/src/org/mmtk/utility/heap/FreeListPageResource.java

But for accuracy as well as licensing reasons, it is better to rewrite the comments accurately according to the code, in the Rust style.

wks avatar May 26 '23 03:05 wks