sysconfig, sizeof: advertise a build without the lock, and report its layout - #1245
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe change updates no-GIL object layout constants and documentation. It aligns ChangesNo-GIL object sizing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR restores free-threaded build naming and corrects object-size and layout reporting, with the supplied checks passing and no actionable merge-blocking risk remaining after normal review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ee4e58d84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let is = |candidate: *const PyType| std::ptr::eq(layout, candidate); | ||
| let (base, item) = if is(&pyre_object::INSTANCE_TYPE) { | ||
| (2 * word, 0) | ||
| (4 * word, 0) |
There was a problem hiding this comment.
On wasm32, which this repository explicitly supports, CPython's in-tree test.support models the free-threaded header as PHBBInP (lib-python/3/test/support/__init__.py:937-946), totaling 20 bytes when pointers and Py_ssize_t are four bytes. This line instead reports 4 * word, or 16 bytes; the same four-word assumption is repeated throughout the changed layout table and str.__sizeof__, so __basicsize__, __sizeof__, and sys.getsizeof are at least four bytes low on 32-bit builds despite advertising Py_GIL_DISABLED=1. Use architecture-correct field-layout sizes rather than a fixed word-count bump.
AGENTS.md reference: AGENTS.md:L249-L254
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 5c04e85). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 653b6c5721
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // object to link lines as `-lpython3.x`. | ||
| store_int(vars, "Py_DEBUG", 0); | ||
| store_int(vars, "Py_GIL_DISABLED", 0); | ||
| store_int(vars, "Py_GIL_DISABLED", 1); |
There was a problem hiding this comment.
Any code branching on sysconfig.get_config_var('Py_GIL_DISABLED') now treats pyre as free-threaded, but majit/majit-gc/src/gc_sync.rs:246-271 still acquires the process-wide lock for every registered runtime thread, and sys.flags.gil remains hard-coded to 1. This enables free-threaded stdlib/test paths and ABI selection even though -X gil=0 is unavailable; retain 0 until the runtime can actually execute without that lock rather than changing the advertised build to make the size checks select another layout.
AGENTS.md reference: AGENTS.md:L231-L237
Useful? React with 👍 / 👎.
| let unload_marker = out_dir.join("unloaded"); | ||
| let source = root.join("pyre/pyrex/tests/fixtures/cpyext_smoke.c"); | ||
| let include = root.join("pyre/pyre-interpreter/include/pyre3.14"); | ||
| let include = root.join("pyre/pyre-interpreter/include/pyre3.14t"); |
There was a problem hiding this comment.
When an extension is compiled against the newly selected pyre3.14t/Python.h, the header never defines Py_GIL_DISABLED, despite the interpreter reporting that config variable as 1. Generated extension code already uses this macro for its ABI sanity check (for example pypy/module/cpyext/test/builtin_min_max.c:8936-8951), so it compiles the non-free-threaded branch and then raises ImportError when loaded because runtime sysconfig claims the opposite ABI.
Useful? React with 👍 / 👎.
… layout #1236 moved `Py_GIL_DISABLED` to 0 and dropped the `t` from `sys.abiflags`, `ABIFLAGS`, `sys.winver` and the installed stdlib directory. Restore the four spellings and `lib/pyre3.14t`, including the cpyext include directory, `stage-stdlib.py`, the `importing.rs` search paths and the `dist-workspace.toml` comment naming the archived directory. The blackhole and residual-call halves of that commit are untouched. The object header such a build carries is four words, not two, and the `tp_basicsize`/`tp_itemsize` projection reported the two-word layout: - `cpython_type_layout`: the entries that grow by two words, plus `type` and `PyWeakReference`, which grow by three. - `cpython_type_offsets`: the `type`, `set`/`frozenset` and `memoryview` inline offsets, and the managed weakref word, which sits two words behind the instance rather than four. - `str.__sizeof__`: PyASCIIObject 7 words, PyCompactUnicodeObject 9, PyUnicodeObject 10. - `type.__sizeof__`: PyHeapTypeObject 120 words, PyTypeObject 54. - `object.__sizeof__`: the fallback for a type with no entry. - `sys.getsizeof`: `_PyType_PreHeaderSize` charges no `PyGC_Head`. Without the lock the collector keeps its bits in the object header, so a tracked instance pays nothing for the wider header while an untracked one pays the full two words. - `list.__sizeof__`: `list_sort_impl` writes -1 into `allocated` while the items are detached; clamp it rather than wrap in `size_t`. `cpython_object_is_gc` loses its only caller and keeps the flag it reads; the comments on it and on `flag_have_gc` no longer claim `getsizeof` adds a collector pre-header. `dict_set_sizeof_python314.py` derives the header width from `Py_GIL_DISABLED` rather than hard-coding it. The parity runner executes each fixture under both the CPython oracle and pyre, and those two now disagree about the build, so one set of constants cannot satisfy both. `test.support.calcobjsize` keys off the same config var. `test.test_interpreters` is already recorded SKIP in `KNOWN_SKIPS` and the baseline by #1253, whose stated reason — the package `__init__` raises `SkipTest("GIL disabled")` when `Py_GIL_DISABLED` — holds again with the advertisement restored. Values measured against CPython 3.14.6 free-threaded: 22 of the 23 types the table covers now answer the same number. `list_reverseiterator` still has no entry and reports 32 against 48; that gap predates this change. `test.test_str` `test_raiseMemError` was the visible failure; the rest of the projection is reached only by `@support.cpython_only` assertions, which pyre skips. Assisted-by: Claude
1 participant
#1236 moved
Py_GIL_DISABLEDto 0 and dropped thetfromsys.abiflags,ABIFLAGS,sys.winverand the installed stdlib directory. This restores thefour spellings and
lib/pyre3.14t, and makes the object-layout projectionreport the header such a build actually carries.
The two halves have to move together:
site.py:409derives the site-packagesdirectory name from
sys.abiflagswhilesysconfigderives the same name fromPy_GIL_DISABLED, so changing one alone installs into a directory the otherdoes not put on
sys.path.scripts/stage-stdlib.pywrites that directory,importing.rsfinds an installed tree by it, andpyrex/tests/cpyext_smoke.rsand the
dist-workspace.tomlcomment name it too.The blackhole and residual-call halves of #1236 are untouched.
The layout half
The object header a build without the lock carries is four words, not two, and
the
tp_basicsize/tp_itemsizeprojection reported the two-word layout — a16-byte disagreement with what the build advertises.
test.test_str::test_raiseMemErroris where that surfaced. Its self-checkcompares
sys.getsizeof(char * 42)against a struct size thatsupport.calcobjsizederives fromPy_GIL_DISABLED, and all four subtests cameout exactly two words short.
cpython_type_layout— the entries that grow by two words, plustypeandPyWeakReference, which grow by three.cpython_type_offsets— thetype,set/frozensetandmemoryviewinlineoffsets, and the managed weakref word, which sits two words behind the
instance rather than four.
str.__sizeof__— PyASCIIObject 7 words, PyCompactUnicodeObject 9,PyUnicodeObject 10.
type.__sizeof__— PyHeapTypeObject 120 words, PyTypeObject 54.object.__sizeof__— the fallback for a type with no entry.sys.getsizeof—_PyType_PreHeaderSizecharges noPyGC_Head. Without thelock the collector keeps its bits in the object header, so a tracked instance
pays nothing for the wider header while an untracked one pays the full two
words. This is why the tracked builtins report the same size either way and
only the leaves move.
list.__sizeof__—list_sort_implwrites -1 intoallocatedwhile theitems are detached; clamp it rather than wrap in
size_t.cpython_object_is_gcloses its only caller and keeps the flag it reads; thecomments on it and on
flag_have_gcno longer claimgetsizeofadds acollector pre-header.
The parity fixture
pyre/extra_tests/parity_tests/dict_set_sizeof_python314.pyhard-coded twelvecontainer sizes taken from a build with the lock. The parity runner executes
each fixture under both the CPython oracle and pyre, and those two now disagree
about the build, so one set of constants cannot satisfy both — it derives the
header width from
Py_GIL_DISABLEDinstead, the same config vartest.support.calcobjsizekeys off. Branching rather than skipping keeps theoracle asserting its own layout.
Every value is
+16(two words) on the free-threaded side, and pyre matches thefree-threaded oracle on all twelve.
test.test_interpretersAlready recorded SKIP in
KNOWN_SKIPSand the baseline by #1253. Its statedreason —
test/test_interpreters/__init__.pyraisesSkipTest("GIL disabled")when
Py_GIL_DISABLED— describes the pre-#1236 build and holds again here.Nothing in this PR changes the baseline.
Verification
Values were measured, not derived: every number comes from
sys.getsizeofunder a CPython 3.14.6 free-threaded build, not from arithmeticon the GIL layout. That mattered —
typeandweakref.refgrow by three words,not two, and the subclass path needed the pre-header term removed as well; both
would have been wrong under arithmetic and neither is visible to the gate,
because those assertions are
@support.cpython_only.Of the 23 types the table covers, 22 now answer the same number as the oracle.
list_reverseiteratorstill has nocpython_type_layoutentry and reports 32against 48; that gap predates this change and is not touched here.
cargo test --all --no-default-features --features dynasmpyre/cpython_tests/run.py(full gate, local)test.test_strparity_tests/run.py --dynasm-only/--cranelift-onlyAlso verified on Linux aarch64 in a container earlier in the branch's life.
Remaining CI red is base-owned
main is red on the same jobs with the same signatures at both
4b3f447dba3and5b658fd871c, and none of it is touched here:CPython suite (gate)—test.test_dataclasses: PASS -> CRASHSIGSEGV intest_classvar_module_level_import, identical on main.pyre/check.py(ubuntu, windows) —dynasm synth/generator_tree_recursionjit-stats
guard_failures 2951 -> 2955, identical on main.pyre/check.pywasm/cranelift ratio gates — timing-driven; main fails asuperset of ours (5 wasm + 1 cranelift against our 4 wasm).
— authored by Claude
Summary by CodeRabbit
__sizeof__()reporting for dictionaries, sets, frozensets, lists, strings, and related built-in objects when running without the GIL.