Open wks opened 1 year ago
Related PR about plan constraints: https://github.com/mmtk/mmtk-core/issues/588
I think it is a mistake.
MAX_INT
is not a meaningful value for anything
- If it is intended to express "no limit" in the sense that the default space can hold objects of any sizes, it should have been
BYTES_IN_ADDRESS
.- If it is giving a reasonable default for plans, I don't think
MAX_INT
is a good default value because a 2GB object is too big for almost anything other than LOS. A reasonable value should be something like "4 * block_size".
It means no limit. The code was just ported from Java MMTk.
https://github.com/mmtk/mmtk-core/pull/1026 made Java specific constants pub(crate)
. So no binding is using those constants right now. We can do refactoring within mmtk-core, and remove the constants.
TL;DR:
src/util/constants.rs
defines many Java-specific constants, such asBYTES_IN_INT
andBYTES_IN_LONG
. Many other places in mmtk-core use those constants. We should replace them with Rust counterparts, or more meaningful constants.Problem
MMTk was ported from JikesRVM written in Java, and Java integer types have precisely defined sizes. The Java MMTk had constants that represent the sizes of those built-in integer types. They are:
{,LOG_}{BITS,BYTES}_IN_{CHAR,SHORT,INT,LONG}
and{MAX,MIN}_INT
.When porting to Rust, we copied those constants over. But those constants are foreign for Rust users because there is no
short
,int
orlong
in Rust, but there arei16
,i32
andi64
instead. Moreover, as a language-agnostic GC framework, MMTk should not be Java-specific. So it doesn't make sense to use those Java-specific constants (i.e. to depend on Java's semantics ofshort
,int
andlong
, etc.).Which parts are using those constants
We can let the compiler tell us which parts of mmtk-core are using those constants by adding
#[deprecated]
to those constants.src/plan/generational/barrier.rs
In
memory_region_copy_slow
, there is an assertionIt was
BYTES_IN_ADDRESS
, but was changed toBYTES_IN_INT
in https://github.com/mmtk/mmtk-core/pull/899. It may be related to CompressedOops.src/plan/plan_constraints.rs
In
PlanConstraints::default()
,max_non_los_default_alloc_bytes
andmax_non_los_copy_bytes
are set toMAX_INT
. They are both thresholds for deciding whether objects should go to LOS. There are separate values for "alloc" and "copy" because when copying, object size may grow (for example, for implementing address-based hashing).I think it is a mistake.
MAX_INT
is not a meaningful value for anythingUSIZE::MAX
.If it is giving a reasonable default for plans, I don't think(update: @qinsoon commented that it is intended for "no limit")MAX_INT
is a good default value because a 2GB object is too big for almost anything other than LOS. A reasonable value should be something like "4 * block_size".src/policy/marksweepspace/malloc_ms/global.rs
The constant
MAX_OBJECT_SIZE
is defined asMAX_INT
, butMAX_OBJECT_SIZE
is unused.src/util/alloc/allocator.rs
The function
fill_alignment_gap
fills alignment gaps oneVM::ALIGNMENT_VALUE
at a time. ButVM::ALIGNMENT_VALUE
is ausize
. This is obvious a bug because that will cause ausize
value to be stored at unaligned addresses, which has undefined behaviour.src/util/heap/space_descriptor.rs
The constant
BASE_EXPONENT
is defined asBITS_IN_INT - MANTISSA_BITS
. It is used increate_descriptor_from_heap_range
to perform bit shifting, and is only used on 32-bit systems. I think it should beu32::BITS
.src/util/raw_memory_freelist.rs
The constant
LOG_ENTRY_BITS
is defined asLOG_BITS_IN_INT
. The freelist data structure from JikesRVM uses 32-bit entries to form a linked list. It should beu32::BITS.ilog2()
.src/vm/mod.rs
The constants
DEFAULT_LOG_{MIN,MAX}_ALIGNMENT
are defined asLOG_BYTES_IN_{INT,LONG}
, respectively. They are just defaults. I think it should be OK to replace them withu32::BITS.ilog2()
andu64::BITS.ilog2()
.