Page based heap size heuristics #50144
Conversation
|
Could you add documentation to https://docs.julialang.org/en/v1/devdocs/gc/ this would also help with other people understanding the trade-offs in the design. |
vchuravy
added
needs docs
|
It does seem like 32 bit is just very near OOM, this PR shouldn't really affect that. |
|
Wtf is up with windows |
gbaraldi
removed
needs docs
|
Am I reading correctly that this PR sets the max heap for 32 bit systems to 1mb? |
|
It does, I'm losing my mind with the 32 bit stuff. |
|
IMO playing with the number more is unlikely to help. If it's running out of address space with a 1GB cap that pretty strongly suggests to me that the algorithm is the problem, not the number. |
|
So the issue here is that the old algorithm didn't actually use the target you set it to, so 2GB for it didn't mean much, while the new algorithm will use that much memory if needed. And it turns out that finding that number is annoying. (You can check the maxrss of the test processes, I'm basically trying to match what we got before. |
vchuravy
changed the title
|
Each GC may thrash the pagefile, so I would think any extra GC is too much |
|
Not sure what you mean by extra GC? This is for the basically hanged condition of using more memory than we allowed. For now if we detect that we now increase the heap using the default behaviour because the rate based ones are probably nonsense in this condition. |
|
I meant that |
|
Oh, I see. The way I implemented it GC thrashing is GC time vs mutator time, the heap size doesn't actually influence it. |
src/gc.c
Outdated
| @@ -3575,7 +3588,7 @@ void jl_gc_init(void) | |||
| if (total_mem < 128e9) | |||
| percent = total_mem * 2.34375e-12 + 0.6; // 60% at 0 gigs and 90% at 128 to not | |||
| else // overcommit too much on memory contrained devices | |||
| percent = 0.9; | |||
| percent = 0.8; | |||
There was a problem hiding this comment.
I think these thresholds are now provably wrong now. For uv_get_total_memory, we want allow allocating 10x that amount before we start worry too much about the OOM killer, otherwise we are just trashing performance by thrashing swap for no reason. For uv_get_constrained_memory, the number should be something like 0.8% - 256MB so that we account for the fixed cost of the system image (roughly) and otherwise try to push right up against the limit (leaving only about 20% for other parts of the program, as a guess).
| jl_atomic_store_relaxed(&gc_heap_stats.heap_size, | ||
| jl_atomic_load_relaxed(&gc_heap_stats.heap_size) - (v->sz&~3)); |
There was a problem hiding this comment.
This seems wrong, you want an atomic decrement, probably?
There was a problem hiding this comment.
Big sweep is single threaded for now.
| uint64_t alloc_acc = jl_atomic_load_relaxed(&ptls->gc_num.alloc_acc); | ||
| if (alloc_acc + sz < 16*1024) | ||
| jl_atomic_store_relaxed(&ptls->gc_num.alloc_acc, alloc_acc + sz); |
There was a problem hiding this comment.
Similary here, you could lose updates if you don't do atomic_inc
There was a problem hiding this comment.
alloc_acc is a per thread value, the global update is a couple lines below with the full fetch_add
This PR implements GC heuristics based on the amount of pages allocated instead of live objects like was done before. The heuristic for new heap target is based on https://dl.acm.org/doi/10.1145/3563323 (in summary it argues that the heap target should have square root behaviour). From my testing this fixes #49545 and #49761 (cherry picked from commit 32aa29f)
Backported PRs: - [x] #50411 <!-- Fix weird dispatch of * with zero arguments --> - [x] #50202 <!-- Remove dynamic dispatch from _wait/wait2 --> - [x] #50064 <!-- Fix numbered prompt with input only with comment --> - [x] #50026 <!-- Store heapsnapshot files in tempdir() instead of current directory --> - [x] #50402 <!-- Add CPU feature helper function --> - [x] #50387 <!-- update newpages pointer after actually sweeping pages --> - [x] #50424 <!-- avoid potential type-instability in _replace_(str, ...) --> - [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some cases --> - [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized --> - [x] #50466 <!-- relax assertion involving pg->nold to reflect that it may be a bit in… --> - [x] #50490 <!-- Fix compat annotation for italic printstyled --> - [x] #50488 <!-- fix typo in `Base.isassigned` with `Tridiagonal` --> - [x] #50476 <!-- Profile: Add specifying dir for `take_heap_snapshot` and handling if current dir is unwritable --> - [x] #50461 <!-- fix typo in the --gcthreads argument description --> - [x] #50528 <!-- ssair: Correctly handle stmt insertion at end of basic block --> - [x] #50533 <!-- ensure internal_obj_base_ptr checks whether objects past freelist pointer are in freelist --> - [x] #49322 <!-- improve cat design / performance --> - [x] #50540 <!-- gc: remove over-eager assertion --> - [x] #50542 <!-- gf: remove unnecessary assert cycle==depth --> - [x] #50559 <!-- Expand kwcall lowering positional default check to vararg --> - [x] #50058 <!-- Add unwrapping mechanism for triangular mul and solves --> - [x] #50551 <!-- typeintersect: also record chained `innervars` --> - [x] #50552 <!-- read(io, Char): fix read with too many leading ones --> - [x] #50541 <!-- precompile: ensure globals are not accidentally created where disallowed --> - [x] #50576 <!-- use atomic compare exchange when setting the GC mark-bit --> - [x] #50578 <!-- gf: make method overwrite/delete an error during precompile --> - [x] #50516 <!-- Fix visibility of assert on GCC12/13 --> - [x] #50597 <!-- Fix memory corruption if task is launched inside finalizer --> - [x] #50591 <!-- build: fix various makefile bugs --> - [x] #50599 <!-- faster invalid object lookup in conservative gc --> - [x] #50634 <!-- 🤖 [master] Bump the SparseArrays stdlib from b4b0e72 to 99c99b4 --> - [x] #50639 <!-- Backport LLVM patches to fix various issues. --> - [x] #50546 <!-- Revert storage of method instance in LineInfoNode --> - [x] #50631 <!-- Shift DCE pass to optimize imaging mode code better --> - [x] #50525 <!-- only check that values are finite in `generic_lufact` when `check=true` --> - [x] #50587 <!-- isassigned for ranges with BigInt indices --> - [x] #50144 <!-- Page based heap size heuristics --> Need manual backport: - [ ] #50595 <!-- Rename ENV variable `JULIA_USE_NEW_PARSER` -> `JULIA_USE_FLISP_PARSER` --> Non-merged PRs with backport label: - [ ] #50637 <!-- Remove SparseArrays legacy code --> - [ ] #50618 <!-- inference: continue const-prop' when concrete-eval returns non-inlineable --> - [ ] #50598 <!-- only limit types in stack traces in the REPL --> - [ ] #50594 <!-- Disallow non-index Integer types in isassigned --> - [ ] #50568 <!-- `Array(::AbstractRange)` should return an `Array` --> - [ ] #50523 <!-- Avoid generic call in most cases for getproperty --> - [ ] #50172 <!-- print feature flags used for matching pkgimage -->
KristofferC
removed
the
backport 1.10
| if (target_allocs == 0.0 || thrashing) // If we are thrashing go back to default | ||
| target_allocs = 2*sqrt((double)heap_size/min_interval); | ||
|
|
||
| uint64_t target_heap = (uint64_t)target_allocs*min_interval + heap_size; |
There was a problem hiding this comment.
this will be equivalent to sqrt(heap_sizealloc_rate/(gc_ratetuning_factor)) * sqrt(min_interval) + heap_size.
I am a bit confused - shouldnt this be independent of min_interval?
There was a problem hiding this comment.
I used it as a scaling factor basically. It made it easier for me to reason about what taking the sqrt meant.
This PR implements GC heuristics based on the amount of pages allocated instead of live objects like was done before. The heuristic for new heap target is based on https://dl.acm.org/doi/10.1145/3563323 (in summary it argues that the heap target should have square root behaviour). From my testing this fixes JuliaLang#49545 and JuliaLang#49761
This PR implements GC heuristics based on the amount of pages allocated instead of live objects like was done before.
The heuristic for new heap target is based on https://dl.acm.org/doi/10.1145/3563323 (in summary it argues that the heap target should have square root behaviour).
From my testing this fixes #49545 and #49761