Conversation
arvidn
force-pushed
the
garbage-collection
branch
2 times, most recently
from
caf4dea to
9df4a48
Compare
Pull Request Test Coverage Report for Build 23138661607Details
💛 - Coveralls |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
arvidn
force-pushed
the
garbage-collection
branch
3 times, most recently
from
f641887 to
99ddcd5
Compare
arvidn
force-pushed
the
garbage-collection
branch
4 times, most recently
from
73275d0 to
d9e0b3e
Compare
|
@cursor review |
arvidn
force-pushed
the
garbage-collection
branch
from
d9e0b3e to
0690684
Compare
arvidn
force-pushed
the
garbage-collection
branch
from
0690684 to
a870ef1
Compare
arvidn
force-pushed
the
garbage-collection
branch
3 times, most recently
from
45db360 to
2118254
Compare
arvidn
force-pushed
the
garbage-collection
branch
2 times, most recently
from
6dcb742 to
fc386c2
Compare
arvidn
force-pushed
the
garbage-collection
branch
from
fc386c2 to
d9b6fa5
Compare
arvidn
force-pushed
the
garbage-collection
branch
2 times, most recently
from
6b54329 to
a2c5124
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
ghost_heap change in new_atom affects all code paths
Medium Severity
Adding self.ghost_heap += v.len() in new_atom for small atoms changes heap_size() and the heap limit check (start as usize + self.ghost_heap + v.len() > self.heap_limit) for ALL callers unconditionally, not just when ENABLE_GC is set. Previously new_atom did not track ghost_heap for small atoms while new_small_number did — this fixes that inconsistency, but it changes the observable heap_size() and makes the OOM check slightly more restrictive for every program. Since heap limits are consensus-critical (especially under LIMIT_HEAP / MEMPOOL_MODE), programs near the limit could now fail where they previously succeeded, potentially causing a consensus divergence between nodes running old vs. new code.
There was a problem hiding this comment.
LIMIT_HEAP and MEMPOOL_MODE are, by definition, not consensus mode.
…ply invocation results in a simple atom being returned. Everything allocated by the invocation can be freed
… program execution
arvidn
force-pushed
the
garbage-collection
branch
from
a2c5124 to
57366f9
Compare
1 participant
This PR implements a simple, call stack-based, garbage collection.
overview
The observation is that any operator that returns an atom that was allocated before the snapshot was taken (i.e. picking an atom out of the environment) or a simple atom. e.g.
NILor1effectively "launder" all computation (and allocations) that went into computing its arguments into a very small amount of data.When this happens we can reset the
Allocatorto the state it was in before we invoked the operator, as long as we preserve the return value.This requires us to pre-emptively store
Allocatorcheckpoints just in case the operator returns a simple atom. This adds overhead and we'll have to make a judgement call on whether we think it's worth it.The cost of recording Allocator snapshots has been mitigated by:
Checkpoint, calledTransparentCheckpointthat's smaller. It usesu32instead ofusizeand it does not record the "ghost" counters.src/chia_dialect.rsingc_candidate().restoring the
AllocatorWhen restoring the allocator, it's important that we preserve the same behavior as we have today, with regards to counters. The atom- and pair counters are consensus critical, since we have upper limits on them. So when we restore the allocator, we have to do it transparently, and preserve counters.
The way this is done is to increment the ghost-counters by the same amount as we are freeing. This acts as if the atoms and pairs were never freed, just like it works today.
This is one important distinction between the existing
checkpoint()->restore_checkpoint()functions and the newtransparent_checkpoint()->restore_transparent_checkpoint()functions. But note that the transparent version is a subset of the existing behavior.benchmarking
To benchmark this I picked some of the most expensive block generators from mainnet. Expensive in the number of atoms, pairs and heap-bytes allocated, but also in execution run-time.
Extending the
analyze-chain.rstool, I picked the generators at the following heights:running these generators before and after this change gave me the following results:
comparison
run-time
peak atom count
peak pair count
peak heap size
before (main)
after (with this change)
Note
High Risk
High risk because it changes core
run_program/allocator behavior and introduces checkpoint/restore paths that affect consensus-critical allocation accounting, even though gated behind the newClvmFlags::ENABLE_GCflag.Overview
Adds an optional, flag-gated garbage-collection mechanism that snapshots the
Allocatorbefore selected operators and may transparently restore it after the operator returns, reducing retained allocations while preserving atom/pair/heap accounting via ghost counters.This introduces
TransparentCheckpointplusmaybe_restore_with_node()inAllocator, wires a newDialect::gc_candidate()hook (implemented inChiaDialectunderClvmFlags::ENABLE_GC) intorun_programvia a newRestoreAllocatoroperation, and updates benchmarks/tools/tests to run withENABLE_GCplus new fuzz coverage comparingENABLE_GCvs non-GC outcomes.Written by Cursor Bugbot for commit 57366f9. This will update automatically on new commits. Configure here.