Repository navigation
Conversation
|
The following commit authors need to sign the Contributor License Agreement: |
Documentation build overview
56 files changed ·
|
|
@daanx can you please sign the CLA? |
Ah yes, but not really for the current PR as that includes the full mimalloc v3 vendored sources and that is a separate project under the MIT license. I can of course sign off for the changes I made on the CPython side. The current PR contains both, maybe we can split it? I think commit cedd47b is the CPython part only, while the other commits are just importing mimalloc v3 as a vendored library. |
daanx
left a comment
There was a problem hiding this comment.
Using MI_WIN_INIT_USE_FLS is not really encouraged for mimalloc. If we can use the default that would be best.
There's a crash on the Windows builds w/o it currently (and it seems to show up basically at startup but the freeze programs make it through it). I was planning on digging into it more later but this is what Claude came up as a fix - I just don't have immediate access to a Windows machine to see where the actual crash is coming from. But that's why it's there for now. Right now I'm just trying to get things to green CI and then I'll clean up the final result. |
|
@corona10 Hey here it is! |
|
About guardrail for memory pressure
|
|
Perf run is here waiting for results: https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/39346123598 |
FWIW the docs due already call out mimalloc license: https://docs.python.org/3/license.html#mimalloc although we should probably update the date range w/ the v3 update. |
2x to 1.14x but still higher than mimalloc v2 |
| } | ||
| } | ||
| // Hooks can run under allocator locks, so only consult cached progress. |
There was a problem hiding this comment.
| // Hooks can run under allocator locks, so only consult cached progress. | |
| if (qsbr != NULL) { | |
| // Scan thread sequences like the v2 integration did. | |
| return !_Py_qsbr_poll(qsbr, page->goal); | |
| } | |
| // Hooks can run under allocator locks, so only consult cached progress. |
That was still a 6% regression on memory (but things are headed in the right direction, THP tweak about it from 2x to 1.15, and then the _Py_qsbr_poll fix got us here), here's another run https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/39476892845 which has commit 3c8be4f that sets |
|
Another run with new options for mi_option_arena_purge_min_size, and mi_option_page_full_retain and a tweaked mi_option_arena_purge_mult: https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/39497633255 |
|
Down to 3% |
|
This is now neutral: https://github.com/facebookexperimental/free-threading-benchmarking/tree/main/results/bm-20261020-3.16.0a1+dev-a36efb6-NOGIL It includes one more upstream fix which is a36efb6 that re-enables the @daanx Thanks for merging all of the other upstream PRs I sent. I can send another one to re-enable this but I can also throw it under another ifdef if you're concerned about having it on in the common case. But I think this basically brings us to parity where we could merge this. I'd still want to bring back the latest commits you have upstream but my previous run had that as neutral. I also am assuming your dev3 branch isn't yet released? |
|
Another benchmark run w/ the latest and greatest mimalloc/dev3 branch merged in: https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/39651451425 We're now down to 4 vendored in changes which I've opened upstream PRs for (the last being the most questionable I think): |
Sync Objects/mimalloc and Include/internal/mimalloc with microsoft/mimalloc dev3 (1ff2382, "fix profile timestamps"). Most of the local edits previously carried in the vendored copy have been merged upstream (forceinline declaration order, WASI page size, quoted includes, MSan syscalls, Emscripten builtin allocator, MI_NO_ALIGN_FALLBACK_WARNING, MI_NO_CXX_NEW_HANDLER, __ImageBase, trailing whitespace). The FLS init assertion fix is dropped as CPython no longer uses MI_WIN_INIT_USE_FLS. Changes still carried on top of dev3 (proposed upstream): - Fix build with MI_NO_THP after the MADV_NOHUGEPAGE change - Fix -Wsign-compare warning when reading hpage_pmd_size - Fix data race on the thread-done pthread key at process exit CPython-specific change: - Purge expired ranges on arena free, as in mimalloc v2, to keep the peak memory of short-running processes at v2 levels. Define MI_NO_ALIGN_FALLBACK_WARNING in pycore_mimalloc.h, since the vendored copy now has the upstream opt-out instead of the warning being commented out.
This reverts commit 8e6401e.
Vendor mimalloc v3 (dev3 branch) into Objects/mimalloc and Include/internal/mimalloc.
|
One more run after merging against and updating the define for 3.6 #160360 https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/41690003577 |
|
It looks like the slice shift tweak hits assertions, doing another perf run w/o it and with the base having it: https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/41693317724 |
|
While looking into #161420, I tried this PR. In #161420, worker threads allocate 16 MiB The v3 code in this PR would fix that, but one of the option defaults the PR sets for CPython turns the fix off. Why v3 fixes it. With its default options, v3 abandons a page that becomes full: the page no longer belongs to the allocating thread ( Why it comes back here. This PR sets Peak memory, with about 150 MiB needed:
Could abandoning stay enabled, at least for pages of large blocks? The C program and all numbers are in this comment on microsoft/mimalloc#1117. |
…e aggressively collected (see also python/cpython#159718)
|
Hi @maikschulze , @DinoV : I agree that using However, we should try again also with the default page_full_retain (2). I think there is a mistake in the current PR: op->ob_tid = mi_page_is_abandoned(page) ? 0 : mi_page_thread_id(page);
if (op->ob_tid == 0) merge_refcount(op, 0);this should become: op->ob_tid = mi_thread_id_of(op); // this gets a CPython thread id (not a mimalloc abandoned page tid) (todo: make this more efficient by avoiding overwriting the tid in the object header (and avoiding this call)?
if (op->ob_tid == 0) merge_refcount(op, 0);edit: actually, it would be even better to call the internal I think with this change we don't need the page_full_retain and can use abandoned pages (which should improve matters). Fingers crossed :-) (not sure if you can easily pull from the latest |
|
Thank you, @daanx! I tried the C version of the example (8 threads, 16 MiB blocks freed by the main thread, about 144 MiB in flight) on the latest
|
|
Good see that @maikschulze; @DinoV , I tested the latest mimalloc dev3 with the fix I proposed: --- a/Python/gc_free_threading.c
+++ b/Python/gc_free_threading.c
@@ -311,8 +311,8 @@ gc_restore_tid(PyObject *op)
op->ob_tid = 0;
}
else {
- // Reclaimed pages may have a new owner; abandoned pages have none.
- op->ob_tid = mi_page_is_abandoned(page) ? 0 : mi_page_thread_id(page);
+ // Full-page abandonment preserves the QSBR thread ownership.
+ op->ob_tid = mi_page_qsbr_thread_id(page);
if (op->ob_tid == 0) {
merge_refcount(op, 0);
}and it all seems to work now with default mimalloc settings, i.e. I used the following (agent generated) test script that tries to probe threads creating abandoned pages that get picked up by other threads and gc'd:import argparse
from collections import Counter
from concurrent.futures import ThreadPoolExecutor
import ctypes
import faulthandler
import gc
import threading
import sys
import weakref
parser = argparse.ArgumentParser()
parser.add_argument("--reclaim", action="store_true")
args = parser.parse_args()
if sys.implementation.name != "cpython" or sys._is_gil_enabled():
raise SystemExit("Requires free-threaded CPython with the GIL disabled")
TIMEOUT = 30
COUNT = 20_000
faulthandler.dump_traceback_later(TIMEOUT, exit=True)
gc.disable()
def object_thread_id(obj):
# ob_tid is the first word of a free-threaded CPython object.
return ctypes.c_size_t.from_address(id(obj)).value
class Node:
__slots__ = ("value", "link", "__weakref__")
def __init__(self, value):
self.value = value
self.link = None
state = {}
ready = threading.Event()
reclaimed = threading.Event()
finish = threading.Event()
def allocate():
objects = [Node(i) for i in range(COUNT)]
state["owner"] = object_thread_id(objects[0])
state["freed"] = {id(objects[i]) for i in range(0, COUNT, 2)}
state["garbage"] = [
weakref.ref(objects[i]) for i in range(0, COUNT, 2)
]
# Half become unreachable cycles, to be freed by GC on another thread.
for i in range(0, COUNT, 2):
objects[i].link = objects[i]
objects[i] = None
state["objects"] = objects
ready.set()
assert finish.wait(TIMEOUT)
return sum(obj.value for obj in objects if obj is not None)
def reclaim_pages():
objects = [Node(i) for i in range(COUNT)]
state["reclaimer"] = object_thread_id(objects[0])
state["reused"] = sum(
id(obj) in state["freed"] for obj in objects
)
state["new"] = objects
reclaimed.set()
assert finish.wait(TIMEOUT)
return sum(
obj.value for obj in state["objects"] if obj is not None
)
expected_sum = sum(range(1, COUNT, 2))
with ThreadPoolExecutor(max_workers=2) as pool:
owner = pool.submit(allocate)
other = None
try:
assert ready.wait(TIMEOUT)
objects = state["objects"]
refs = [
weakref.ref(obj) for obj in objects if obj is not None
]
assert {
object_thread_id(obj)
for obj in objects if obj is not None
} == {state["owner"]}
# Free cycles outside their allocating thread, avoiding owner reclaim.
gc.collect()
assert all(ref() is None for ref in state["garbage"])
expected_owners = {state["owner"]}
if args.reclaim:
other = pool.submit(reclaim_pages)
assert reclaimed.wait(TIMEOUT)
assert state["reused"] > 0, "No freed slots were reused"
expected_owners.add(state["reclaimer"])
for _ in range(2):
gc.collect()
owners = Counter(
object_thread_id(obj)
for obj in objects if obj is not None
)
assert owners.keys() <= expected_owners, owners
if args.reclaim:
assert owners[state["reclaimer"]] > 0, owners
assert sum(
obj.value for obj in objects if obj is not None
) == expected_sum
finally:
finish.set()
assert owner.result() == expected_sum
if other is not None:
assert other.result() == expected_sum
del objects
state.clear()
gc.collect()
assert all(ref() is None for ref in refs)
faulthandler.cancel_dump_traceback_later()
print("PASS:", "reclaimed pages" if args.reclaim else "abandoned pages")To run both scenarios with your free-threaded interpreter: for retain in 0 2; do
MIMALLOC_PAGE_FULL_RETAIN=$retain MIMALLOC_PAGE_RECLAIM_ON_FREE=0 \
./python -X gil=0 qsbr_gc_test.py
MIMALLOC_PAGE_FULL_RETAIN=$retain MIMALLOC_PAGE_RECLAIM_ON_FREE=0 \
./python -X gil=0 qsbr_gc_test.py --reclaim
done |
|
Thanks, I reran the #161420 pattern against this PR, in both directions, in case it helps with the On speed: 2.5.2 makes an
On memory, the pattern: 16 MiB
Reproducer (standard library only)# Large blocks made on one thread and freed on another, 8 in flight: 10 rounds of 64 blocks of 16 MiB.
# usage: python pool_repro.py produce|consume new|keep [gc]
# produce: workers make the blocks, the main thread frees them (as in gh-161420)
# consume: the main thread reads the blocks from a file, workers checksum and free them
# new: a new ThreadPoolExecutor per round (the workers exit); keep: one pool for all rounds
# gc: call gc.collect() after every round
import gc, resource, sys, zlib
from collections import deque
from concurrent.futures import ThreadPoolExecutor
direction, mode, collect = sys.argv[1], sys.argv[2], sys.argv[3:] == ["gc"]
BLOCK, COUNT, THREADS, ROUNDS = 16 << 20, 64, 8, 10
pattern = bytes(range(256)) * (BLOCK // 256)
source = open("/dev/zero", "rb", buffering=0)
def make():
return memoryview(pattern).tobytes() # a new bytes object, as decompress() returns
kept = ThreadPoolExecutor(THREADS) if mode == "keep" else None
for _ in range(ROUNDS):
pool = kept or ThreadPoolExecutor(THREADS)
pending = deque()
for _ in range(COUNT):
if direction == "produce":
pending.append(pool.submit(make)) # the block is freed on the main thread
else:
pending.append(pool.submit(zlib.crc32, source.read(BLOCK))) # freed on the worker
if len(pending) >= THREADS:
pending.popleft().result()
while pending:
pending.popleft().result()
if kept is None:
pool.shutdown() # the workers exit
if collect:
gc.collect()
peak = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
peak = peak >> 20 if sys.platform == "darwin" else peak >> 10
print(f"{direction} {mode}{' gc' if collect else ''}: peak RSS {peak} MiB")for retain in -1 2; do
for args in "produce keep" "consume keep" "produce new" "produce new gc" "consume keep gc"; do
MIMALLOC_PAGE_FULL_RETAIN=$retain ./python pool_repro.py $args
done
doneA probe that suggests the waiting pages are the whole difference (for tracing only, since it lets memory be reused while lock-free readers may still look at it): returning false at the top of if (!mi_heap_qsbr_is_enabled(heap)) return false;
+ return false; // PROBE ONLY: never delay empty pages for QSBR (unsafe in general)
const bool delayed = mi_page_qsbr_should_delay(&page->qsbr, &heap->qsbr, page->capacity * mi_page_block_size(page));The counters are a small patch I can share if useful. Separately: in v3, a block above 31 MiB seems to be purged on every free. As far as I can see, I used an AI assistant to run these measurements and draft this comment; I reviewed the results myself. |
|
Thanks @maikschulze -- I pushed an update to the latest |
This upgrades mimalloc to v3 - the initial work here was done by @daanx and this pushes it over the edge to getting all CI passing and conforming we're at parity with the existing implementation. v3 brings with it baked in qsbr support and it has a number of the CPython specific changes upstreamed now.
To maintain parity with v2 on the pyperformance benchmarks we set several options back to their v2 levels in
pycore_mimalloc.h.There's a new Github CI job added which took 15 seconds to run on this job that verifies we have now modified mimalloc. It is modeled after the
expatrefresh script / CI job. We sync to a specific mimalloc tag and have the hash recorded. It downloads and verifies that the mimalloc vendored code (which lives inObjects/mimallocandInclude/internal/mimalloc) is unchanged.