Simplify minor heaps configuration logic and masking #422
Conversation
There was a problem hiding this comment.
There is a bug with Is_block_and_is_young that will need fixing.
Having looked at the trunk setting for Minor_heap_max which is (1 << 28); that is 268M words or 2GB per domain minor heap on a 64bit platform. I'm wondering if we need the commit that allows overriding of the minor heap limit with OCAMLRUNPARAM:
9494796
Wouldn't it be easier to drop that override commit and settle with (1 << 28)? I'm thinking if people want to experiment with larger than (1<<28) they can change that setting in the header fairly easily and recompile.
Dropping the override commit, it makes us a bit closer to trunk. I'm in favour of dropping the override commit.
runtime/caml/config.h
Outdated
|
|
||
| /* An entire minor heap must fit inside one region | ||
| of size 1 << Minor_heap_align_bits, which determines | ||
| the maximum size of the heap */ | ||
| #if SIZEOF_PTR <= 4 |
There was a problem hiding this comment.
Trunk has
/* Maximum size of the minor zone (words).
Must be greater than or equal to [Minor_heap_min].
*/
#define Minor_heap_max (1 << 28)(1<<28) words is ~2GB per minor heap on a 64bit machine, I think we should just take that.
runtime/caml/mlvalues.h
Outdated
| #define Is_minor(val) \ | ||
| ((((uintnat)(val) ^ (uintnat)Caml_state) & Minor_val_bitmask) == 0) | ||
| #define Is_young(val) \ | ||
| ((char *)(val) < (char *)caml_minor_heaps_end && \ |
There was a problem hiding this comment.
Can we add CAMLassert (Is_block (val)) to the assert?
This will mirror ocaml/ocaml:
#define Is_young(val) \
(CAMLassert (Is_block (val)), \
(char *)(val) < (char *)Caml_state_field(young_end) && \
(char *)(val) > (char *)Caml_state_field(young_start))
runtime/caml/mlvalues.h
Outdated
| ((((uintnat)(val) ^ (uintnat)Caml_state) & Minor_val_bitmask) == 0) | ||
| #define Is_young(val) \ | ||
| ((char *)(val) < (char *)caml_minor_heaps_end && \ | ||
| (char *)(val) > (char *)caml_minor_heaps_base) | ||
|
|
||
| #define Is_block_and_young(val) Is_young(val) |
There was a problem hiding this comment.
This macro needs updating to:
Is_block(val) && Is_young(val)
previously with the bit twiddling Is_young(val) would also tell you if val was a block, but with the lo/hi we need to explicitly check the block as well.
abbysmal
force-pushed
the
engil/refactor_minor_heaps_allocation
branch
from
b1a8b09
to
365efb1
Compare
abbysmal
force-pushed
the
engil/refactor_minor_heaps_allocation
branch
from
3c20ec8
to
70c65bd
Compare
|
I think this PR is ready to be merged if everyone agrees with it. Here are the current results, comparing a few different version (stock, stock+256M, multicore, multicore+default (this patch + default minor heap size), and multicore+256M.) Normalized (stock)
Non normalized
|
|
I think this one is good to go. |
This commit refactor the minor heaps allocation process by first rewriting the logic to not rely on bitmasks and instead move it to a model closer to trunk, introducing a Minor_heap_max configuration variable. Some bits are removed: the arm64 backend used to expose Minor_heap_align_bits in config.h to the emitter, which is removed for now. The Is_minor/Is_young changes are now unified as only Is_young (is the value in *any* minor heap at all?) matters. In a follow up commit, Is_minor will be renamed definitely to Is_yound to follow suit.
abbysmal
force-pushed
the
engil/refactor_minor_heaps_allocation
branch
from
70c65bd
to
d38cfb8
Compare
…il/refactor_minor_heaps_allocation Simplify minor heaps configuration logic and masking
This PR is a first step toward Domain local allocation buffers. (see also #398).
This first step aimed to rewrite the previous bit-twiddling logic of
Minor_heap_sel_bitsandMinor_heap_align_bitswith a scheme closer to trunk.The commits are split by duty, starting with the first one is likely the best.
The first commit introduce
Minor_heap_max, and used this potential maximum size to reserve the minor heaps area.The minor heaps area is reserved for Max_domains (so
Max_domains * Minor_heap_max).Further refactoring is done by removing
Is_minorand only relying onIs_young, which is now a simple bound check (minor_heaps_base < v < minor_heaps_end). In parallel minor I believe such distinction is not required anymore.This specific transition is done in the second commit however.
The third commit introduces a way to override the
Minor_heap_maxsetting usingOCAMLRUNPARAM=s.This may not be the prettiest solution, and does not align with trunk (
Minor_heap_maxshould not be exceeded by the user-configured minor heap.)Sandmark run: