[issue40255] Fixing Copy on Writes from reference counting and immortal objects
Eddie Elizondo added the comment: It seems that we are back on track with perf being back to neutral! I've created 4 new PRs. Each with an optimization applied on top of the baseline introduction of instance immortalization. The main PR 19474 currently stands at around 4%, after rebasing past Eric's PR 30928 it went down to 3%. This is the list of optimizations that I used to get some performance back: * Immortalizing Interned Strings (PR 31488): 1% regression * Immortalizing Runtime Heap After Startup (PR 31489): 0% regression * Immortalizing Modules After Import (PR 31490): 1% regression * Combined Optimizations (PR 31491): 0% improvement All the PRs contain the results of the pyperformance benchmarks and they should each stand on their own in case we want to go for a subset of these optimizations rather than all of them. Make sure to look at each PR to read the implementation details. For testing, in every PR I made sure all the tests were passing on my local environment. Though I do expect some failures happening in non-linux environments. I'll be fixing these over the next couple of days. -- ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting and immortal objects
Change by Eddie Elizondo : -- pull_requests: +29621 pull_request: https://github.com/python/cpython/pull/31491 ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting and immortal objects
Change by Eddie Elizondo : -- pull_requests: +29620 pull_request: https://github.com/python/cpython/pull/31490 ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting and immortal objects
Change by Eddie Elizondo : -- pull_requests: +29619 pull_request: https://github.com/python/cpython/pull/31489 ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting and immortal objects
Change by Eddie Elizondo : -- pull_requests: +29618 pull_request: https://github.com/python/cpython/pull/31488 ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting and immortal objects
Eddie Elizondo added the comment: @eric.snow great to hear about this update! I'll start looking at some of the techniques that we talked about to improve performance, I'm optimistic that we'll be able to close down the gap to 2%. -- ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting
Eddie Elizondo added the comment: > which would potentially save the two word per object overhead Btw, I misread. I thought `gc_bits` referred to the bits used by the GC in the reference count. In any case, you can still use a bit in the reference count :) -- ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting
Eddie Elizondo added the comment:
> I'm somewhat puzzled how a version that does no more work and has no jumps is
> slower.
Oh I think I know why there's some confusion. I've updated the PR from the
initial version (which is probably the one that you saw). The branching does
less work in Py_INCREF and Py_DECREF for all instances marked with the immortal
bit by exiting early.
In the latest change, I added the immortal bit to a bunch of "known" immortal
objects by. For instance:
* All static types (i.e: PyType_Type, etc.)
* All small ints (-5 to 256)
* The following Singletons: PyTrue, PyFalse, PyNone
* And the heap after the runtime is initialized (in pymain_main)
Example 1)
```
PyObject _Py_NoneStruct = {
_PyObject_EXTRA_INIT
1, &_PyNone_Type
#ifdef Py_IMMORTAL_OBJECTS
_Py_IMMORTAL_BIT,
#else
1,
#endif /* Py_IMMORTAL_OBJECTS */
&_PyNone_Type
};
```
Example 2)
```
static int
pymain_main(_PyArgv *args)
{
PyStatus status = pymain_init(args);
if (_PyStatus_IS_EXIT(status)) {
pymain_free();
return status.exitcode;
}
if (_PyStatus_EXCEPTION(status)) {
pymain_exit_error(status);
}
#ifdef Py_IMMORTAL_OBJECTS
/* Most of the objects alive at this point will stay alive throughout the
* lifecycle of the runtime. Immortalize to avoid the GC and refcnt costs */
_PyGC_ImmortalizeHeap();
#endif /* Py_IMMORTAL_OBJECTS */
return Py_RunMain();
```
Therefore, you are now making Py_INCREF and Py_DECREF cheaper for things like
`Py_RETURN_NONE` and a bunch of other instances.
Let me know if that explains it! I could also send you patch of the branch-less
version so you can compare them.
> but making the object header immutable prevents changes like
Why would it prevent it? These changes are not mutually exclusive, you can
still have an immortal bit by:
1) Using a bit from `gc_bits`. Currently you only need 2 bits for the GC. Even
with a `short` you'll have space for the immortal bit.
2) Using a bit from the ob_refcnt. Separately, using this allows us to
experiment with a branch-less and test-less code by using saturated adds. For
example:
```
/* Branch-less incref with saturated add */
#define PY_REFCNT_MAX ((int)(((int)-1)>>1))
#define _Py_INCREF(op) ({
__asm__ (
"addl $0x1, %[refcnt]"
"cmovol %[refcnt_max], %[refcnt]"
: [refcnt] "+r" (((PyObject *)op)->ob_refcnt)
: [refcnt_max] "r" (PY_REFCNT_MAX)
);})
```
--
___
Python tracker
<https://bugs.python.org/issue40255>
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting
Eddie Elizondo added the comment: Mark: > You are asking the whole world to take a hit on both performance and memory > use. That's an explicit non-goal. That's why the code was guarded to add as an optional compilation mode This should be added by default if and only if this is a neutral or an improvement on performance. Which by the way, the latest perf numbers suggest that we are now at perf parity / within error (pending official confirmation from running this on speed.python.org machines). Also, can you expand on how is this a performance hit on memory? > There is only one reference to copy-on-write in a comment. Yet this issue > about making object headers immutable. The PR summary expands on Copy on Writes. If you think this belongs in-code, let me know and I can update the PR. > then make the obvious improvement of changing the branchy code That is strictly slower. The current version makes Py_INCREF and Py_DECREF cheaper for known immortal instances (i.e the heap after runtime init). This skips increasing and decreasing the refcount as well as the refcount check for deallocation. Using the proposed branch-less version makes all Py_INCREFs and Py_DECREFs more expensive. I ran a couple of benchmarks with the branch-less version to highlight this: Branch-less version: unpack_sequence: Mean +- std dev: 98.2 ns +- 4.9 ns richards: Mean +- std dev: 130 ms +- 5 ms fannkuch: Mean +- std dev: 894 ms +- 18 ms Branch version: unpack_sequence: Mean +- std dev: 82.7 ns +- 3.9 ns richards: Mean +- std dev: 123 ms +- 4 ms fannkuch: Mean +- std dev: 838 ms +- 25 ms > Immortality has advantages because it allows saturating reference counting > and thus smaller object headers, but it is not the same as making the object > header immutable. In its current form, this change is not trying to do a strict immutability of the header. We can't guarantee that third-party extensions won't mutate the instance. Instead, this change tries to maintain an instance's immutability as long as it can. If the issue is with naming, I can easily rename this to something else :) -- ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting
Eddie Elizondo added the comment: After the added optimizations and assuming that running this more rigorously (i.e PGO + LTO + full cpu isolation + speed.python.org machine) also shows the PR as perf neutral, what are people's thoughts so far? Would this be enough to consider adding this change by default? Are there still any concerns regarding correctness? It seems like most of the questions raised so far have already been addressed -- ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting
Eddie Elizondo added the comment: Neil: > The fastest would be to create an immortal block as part of the BSS > (uninitialized data). That's an interesting idea, definitely worth exploring and we can probably get some perf win out of it. And yes, using the frozen modules is definitely a step forward and we can leverage that to move these instances into the rodata section of the binary. > I had started doing an experiment with the arena approach before I noticed > Eddie's comment about it. I would like to see his version. What I had written up is slightly different from what you mentioned. I was mostly concerned about having a small object that we did not reach through the GC roots. If this small object would get a reference count bump, the whole arena would Copy on Write. I added a commit to the PR with this Arena Immortalization so that you could take a look at the implementation: https://github.com/python/cpython/pull/19474/commits/b29c8ffd3faf99fc5c9885d2a4c6c3c6d5768c8c The idea is to walk all the arena's pools to mark them as immortal by using a new word added to pool_header. This word would help us identify if the pool is immortal on every pyalloc and pydealloc. I still get some tests breaking with this, I haven't tried to debug it though -- ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting
Eddie Elizondo added the comment: I was able to get an equal (maybe slightly better) performance by following the advice from Steve and Neil and skipping reference counting of instances that we know are immortal. It seems that by getting performance parity, we should be able to ease most of the concerns raised so far. I've updated the PR to now immortalize: * All static types (i.e: PyType_Type, etc.) * All small ints (-5 to 256) * The following Singletons: PyTrue, PyFalse, PyNone * And the heap after the runtime is initialized (in pymain_main) A quick caveat, immortalizing the runtime heap caused ~6 or so tests to fail since they depend on shutdown behavior which this now changes. In the PR I commented out the call to `_PyGC_ImmortalizeHeap` in `pymain_main` to pass the CI. Ignoring these failing tests for now, these are the benchmarks numbers that I got from pyperformance: Baseline (master branch): unpack_sequence: Mean +- std dev: 76.0 ns +- 4.9 ns richards: Mean +- std dev: 116 ms +- 8 ms fannkuch: Mean +- std dev: 764 ms +- 24 ms pidigits: Mean +- std dev: 261 ms +- 7 ms Immortalizing known immortals objects (Latest PR) unpack_sequence: Mean +- std dev: 74.7 ns +- 5.1 ns richards: Mean +- std dev: 112 ms +- 5 ms fannkuch: Mean +- std dev: 774 ms +- 24 ms pidigits: Mean +- std dev: 262 ms +- 11 ms Only adding immortal branch (The commit that Pablo benchmarked) unpack_sequence: Mean +- std dev: 93.1 ns +- 5.7 ns richards: Mean +- std dev: 124 ms +- 4 ms fannkuch: Mean +- std dev: 861 ms +- 26 ms pidigits: Mean +- std dev: 269 ms +- 7 ms The performance of Immortal Objects by default seems to now be within error of the master branch. -- ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting
Eddie Elizondo added the comment:
> the CPU performance implications of adding a branch instruction to Py_INCREC
> and Py_DECREF were, unsurprisingly, quite high.
Yeah, makes sense. I guess it really depends on the specific profile of your
application.
For Instagram this was an overall net positive change and we still have it in
prod. The amount of page faults from Copy on Writes was so large that reducing
it resulted in a net CPU win (even with the added branching). And of course, a
huge reduction in memory usage.
> Microbenchmarks don't tell a good story, the python performance test suite
> should
Agreed. I only added the Richards benchmark as a reference. I'm hoping someone
can pick it up and have more concrete numbers on an average Python workload.
> Given that most people's applications don't fork workers, I do not expect to
> see such an implementation ever become the default.
In any case, I gated this change with ifdefs. In case we don't have it by
default, we can always can easily enable it with a simple
`-DPy_IMMORTAL_INSTANCES` flag to the compiler.
> Also note that this is an ABI change as those INCREF and DECREF definitions
> are intentionally public .h file
This has indeed been a bit of a pain for us as well. Due to how our tooling is
set-up, there's a small number of third party libraries that are still causing
Copy on Writes. Fortunately, that's the only drawback. Even if your
immortalized object goes through an extension that has a different
Py_{DECREF,INCREF} implementation, the reference count will still be so large
that it will never be deallocated.
--
___
Python tracker
<https://bugs.python.org/issue40255>
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +18829 stage: -> patch review pull_request: https://github.com/python/cpython/pull/19474 ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue40255] Fixing Copy on Writes from reference counting
New submission from Eddie Elizondo : Copy on writes are a big problem in large Python application that rely on multiple processes sharing the same memory. With the implementation of `gc.freeze`, we've attenuated the problem by removing the CoW coming from the GC Head. However, reference counting still causes CoW. This introduces Immortal Instances which allows the user to bypass reference counting on specific objects and avoid CoW on forked processes. Immortal Instances are specially useful for applications that cache heap objects in shared memory which live throughout the entire execution (i.e modules, code, bytecode, etc.) -- components: Interpreter Core messages: 366216 nosy: eelizondo priority: normal severity: normal status: open title: Fixing Copy on Writes from reference counting versions: Python 3.9 ___ Python tracker <https://bugs.python.org/issue40255> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38076] Make struct module PEP-384 compatible
Eddie Elizondo added the comment: > I'm concerned by release blocker because 3.9.0a3 version is supposed to be > released soon, and usually release blocker do block a release :-) Ah! That makes sense! In any case, feel free to ping me if you need help on my side to get this PR through (or to remove the release blocker). -- ___ Python tracker <https://bugs.python.org/issue38076> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38076] Make struct module PEP-384 compatible
Eddie Elizondo added the comment: The PR that I sent out already fixes the issue. @vstinner, @pablogsal, please take a look again https://github.com/python/cpython/pull/18039 That should close this issue, no need to work around the bug priority. -- ___ Python tracker <https://bugs.python.org/issue38076> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38076] Make struct module PEP-384 compatible
Eddie Elizondo added the comment: Hey all, I've got a fix for this bug and the CI is green: https://github.com/python/cpython/pull/18039 TL;DR: The module state have to be cleared at a later time. I explain in detail in the PR. Also, I didn't add a new test since there was a test that was already checking for module states in `io`. I added a short comment on it in the PR as well. Otherwise, lmk and I can create a new test for it. -- ___ Python tracker <https://bugs.python.org/issue38076> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38076] Make struct module PEP-384 compatible
Change by Eddie Elizondo : -- pull_requests: +17438 pull_request: https://github.com/python/cpython/pull/18039 ___ Python tracker <https://bugs.python.org/issue38076> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35381] posixmodule: convert statically allocated types (DirEntryType & ScandirIteratorType) to heap-allocated types
Eddie Elizondo added the comment: PR with fix is out. -- ___ Python tracker <https://bugs.python.org/issue35381> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38803] test_wait3 and test_wait4 leaked references on x86 Gentoo Refleaks 3.x
Eddie Elizondo added the comment: Victor, the PR with the fix is out. Easy catch after running it with my leak detector! -- ___ Python tracker <https://bugs.python.org/issue38803> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38803] test_wait3 and test_wait4 leaked references on x86 Gentoo Refleaks 3.x
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +16856 stage: -> patch review pull_request: https://github.com/python/cpython/pull/17373 ___ Python tracker <https://bugs.python.org/issue38803> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35381] posixmodule: convert statically allocated types (DirEntryType & ScandirIteratorType) to heap-allocated types
Eddie Elizondo added the comment: Woops! I'll get to it before the end of the weekend! -- ___ Python tracker <https://bugs.python.org/issue35381> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38803] test_wait3 and test_wait4 leaked references on x86 Gentoo Refleaks 3.x
Eddie Elizondo added the comment: Woops! I'll get to it before the end of the weekend! -- nosy: +eelizondo ___ Python tracker <https://bugs.python.org/issue38803> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38140] Py_tp_dictoffset / Py_tp_finalize are unsettable in stable API
Eddie Elizondo added the comment: Woops, just realized that you already documented this, thanks! Btw, Victor already merged a fix for the windows compiler warning. This issue can be closed now -- ___ Python tracker <https://bugs.python.org/issue38140> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38140] Py_tp_dictoffset / Py_tp_finalize are unsettable in stable API
Eddie Elizondo added the comment: Hey Petr, I'll get to document this and fix the windows warning over the weekend. I'll ping you on Github once it's ready -- nosy: +eelizondo ___ Python tracker <https://bugs.python.org/issue38140> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38152] AST change introduced tons of reference leaks
Eddie Elizondo added the comment: The PR has been merged so the issue can be closed now -- ___ Python tracker <https://bugs.python.org/issue38152> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38152] AST change introduced tons of reference leaks
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +15738 stage: -> patch review pull_request: https://github.com/python/cpython/pull/16127 ___ Python tracker <https://bugs.python.org/issue38152> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38152] AST change introduced tons of reference leaks
Eddie Elizondo added the comment: I have a fix for this coming up. -- nosy: +eelizondo ___ Python tracker <https://bugs.python.org/issue38152> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38150] test_capi: test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_once() leaks
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +15730 stage: -> patch review pull_request: https://github.com/python/cpython/pull/16115 ___ Python tracker <https://bugs.python.org/issue38150> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38150] test_capi: test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_once() leaks
Eddie Elizondo added the comment: > Checking for refleak takes between 2 and 6 hours. Ouch! Makes sense then. We could potentially add a `pre-merge` job that only runs once the merge starts to get executed. Anyways, that's a conversation for another time :-) --- Thanks Stephane I'm very aware of that, I just thought that the build bots did that automatically at PR time. I rather have those drive the signal and letting my slow machine run for hours! Anyways, I'll just have to slightly change my workflow now -- nosy: -matrixise ___ Python tracker <https://bugs.python.org/issue38150> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38150] test_capi: test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_once() leaks
Eddie Elizondo added the comment: On it. Also, I thought that the PR build bots already ran refleak tests by default? Do you know why this it's not integrated to the PR flow? -- nosy: +eelizondo ___ Python tracker <https://bugs.python.org/issue38150> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34533] Apply PEP384 to _csv module
Change by Eddie Elizondo : -- pull_requests: +15700 pull_request: https://github.com/python/cpython/pull/16078 ___ Python tracker <https://bugs.python.org/issue34533> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue38140] Py_tp_dictoffset / Py_tp_finalize are unsettable in stable API
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +15697 stage: -> patch review pull_request: https://github.com/python/cpython/pull/16076 ___ Python tracker <https://bugs.python.org/issue38140> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35381] posixmodule: convert statically allocated types (DirEntryType & ScandirIteratorType) to heap-allocated types
Change by Eddie Elizondo : -- pull_requests: +15533 pull_request: https://github.com/python/cpython/pull/15892 ___ Python tracker <https://bugs.python.org/issue35381> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37879] Segfaults in C heap type subclasses
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +15040 stage: -> patch review pull_request: https://github.com/python/cpython/pull/15323 ___ Python tracker <https://bugs.python.org/issue37879> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37879] Segfaults in C heap type subclasses
New submission from Eddie Elizondo : `subtype_dealloc` is not correctly handling the reference count of c heap type subclasses. It has some builtin assumptions which can result in the type getting its reference count decreased more that it needs to be, leading to its destruction when it should still be alive. Also, this bug is a blocker for the full adoption of PEP384. The full details of the bug along with a fix and tests are described in the Github PR. -- messages: 349905 nosy: eelizondo priority: normal severity: normal status: open title: Segfaults in C heap type subclasses ___ Python tracker <https://bugs.python.org/issue37879> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue36531] PyType_FromSpec wrong behavior with multiple Py_tp_members
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +12617 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue36531> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue36531] PyType_FromSpec wrong behavior with multiple Py_tp_members
New submission from Eddie Elizondo : If a user accidentally defined more than one Py_tp_members in the spec, PyType_FromSpec will ignore all but the last use case. However, the number of members count will cause the type to allocate more memory than needed. This leads to weird behavior and crashes. The solution is a one line fix to just restart the count if multiple Py_tp_members are defined. -- messages: 339468 nosy: eelizondo priority: normal severity: normal status: open title: PyType_FromSpec wrong behavior with multiple Py_tp_members ___ Python tracker <https://bugs.python.org/issue36531> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization does not incref Heap-allocated Types
Eddie Elizondo added the comment: > could we just remove the whole concept of heap allocated types? I do have plans to start migrating more and more CPython modules to use heap allocated types. For example I have posixmodule up in the queue (PR10854) and a local change with all of _io migrated. I'm only blocked by having correct refcnts in these types. So, yes, let's work towards migrating all the static types to heap-allocated types! I have the time and energy to embark on this huge task so you'll see more and more of these PRs from me in the future. :) > First let's make heap types more usable and bug-free, and then it will be > easier In that way, I agree with Petr, let's start by fixing the core issues first. With all of that in mind, it seems to me that we are all agree on the current solution. Let's try to push this forward and merge the PR. -- ___ Python tracker <https://bugs.python.org/issue35810> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization does not incref Heap-allocated Types
Eddie Elizondo added the comment: Bump to get a review on the PR: https://github.com/python/cpython/pull/11661. I believe all comments have now been addressed as well as adding a porting to 3.8 guide. -- ___ Python tracker <https://bugs.python.org/issue35810> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization does not incref Heap-allocated Types
Eddie Elizondo added the comment: > Please open a thread on python-dev Done! https://mail.python.org/pipermail/python-dev/2019-February/156322.html > Yes. You should add a new "Changes in the C API" Done as well, I also included examples for the scenarios that will need fixing to avoid having immortal types: https://github.com/python/cpython/pull/11661 Looking forward to seeing this through! -- ___ Python tracker <https://bugs.python.org/issue35810> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization Bug with Heap-allocated Types
Eddie Elizondo added the comment: Now, with that in mind - can you guys point me to the right thing to do now - how can we move this forward? :) * Should I write something up in python-dev/Discourse? * Do I need to update the PY_VERSION_HEX? * Do I need to write an entry in the Porting guide? Let me know what you think! -- ___ Python tracker <https://bugs.python.org/issue35810> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization Bug with Heap-allocated Types
Eddie Elizondo added the comment:
Thanks for the thorough feedback Stefan!
I would like to just add an extra thing to everything you already mentioned:
This change will NEVER cause a crash. The change that we are introducing is an
incref to a type object (no decrefs). Thus, there are two-ish scenarios:
1) The type object is immortal: This means that the type does not incref/decref
its refcount automatically on instance creation. Adding this incref will not
affect the fact that it's already immortal. An example of this is
structsequences. The change that I added in the PR is to convert it to an
refcounted type (instead of immortal).
2.1) The type is recounted (automatically): Achieved through the generic
allocation which already increfs the type. Given that this refactors that
incref, then this behavior should stay exactly the same.
2.2) The type is refcounted (manually): Achieved by not using the generic
allocation and instead using `PyObject_{,GC}_New{Var}`. To properly refcount
this type, a manual incref is required after object instantiation. Usually,
I've seen this pattern in very carefully engineered types where a NULL is
jammed into `tp_new` to make it into a non-instantiable type. Examples of this
are Modules/_tkinter.c and Modules/_curses_panel.c. Anyways, adding this incref
will leak this type, but will not cause a crash.
--
___
Python tracker
<https://bugs.python.org/issue35810>
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization Bug with Heap-allocated Types
Eddie Elizondo added the comment: Hi Petr, Please take a look at the Github PR. What do you think that's missing to move this forward? I'd be more than happy to add more documentation/testing to it. Let me know what you think -- ___ Python tracker <https://bugs.python.org/issue35810> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization Bug with Heap-allocated Types
Change by Eddie Elizondo : -- keywords: +patch, patch, patch pull_requests: +11461, 11462, 11463 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue35810> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization Bug with Heap-allocated Types
Change by Eddie Elizondo : -- keywords: +patch, patch pull_requests: +11461, 11462 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue35810> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization Bug with Heap-allocated Types
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +11461 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue35810> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35810] Object Initialization Bug with Heap-allocated Types
New submission from Eddie Elizondo :
Heap-allocated Types initializing instances through `PyObject_{,GC}_New{Var}`
will *NOT* not have their refcnt increased. This was totally fine under the
assumption that static types are immortal. However, heap-allocated types MUST
participate in refcounting. Furthermore, their deallocation routine should also
make sure to decrease their refcnt to provide the incref/decref pair.
--
components: Library (Lib)
messages: 334271
nosy: eelizondo
priority: normal
severity: normal
status: open
title: Object Initialization Bug with Heap-allocated Types
versions: Python 3.8
___
Python tracker
<https://bugs.python.org/issue35810>
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35438] Cleanup extension functions using _PyObject_LookupSpecial
Eddie Elizondo added the comment: I also fixed the title to properly reflect what this is trying to achieve. -- ___ Python tracker <https://bugs.python.org/issue35438> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35438] Cleanup extension functions using _PyObject_LookupSpecial
Change by Eddie Elizondo : -- title: Extension modules using non-API functions -> Cleanup extension functions using _PyObject_LookupSpecial ___ Python tracker <https://bugs.python.org/issue35438> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35438] Extension modules using non-API functions
Eddie Elizondo added the comment: @vstinner: Sorry for not being clear - The intention of this change is two-fold: 1) Simplify the implementation of these functions. 2) Reduce the surface area of the C-API. Given that the same functionality can be achieved with public functions of the C-API. This is a nice cleanup over the current approach. -- ___ Python tracker <https://bugs.python.org/issue35438> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35438] Extension modules using non-API functions
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +10316 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue35438> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35438] Extension modules using non-API functions
Eddie Elizondo added the comment: Correction, this is not as trivial as just using `PyObject_GetAttrString`. Will investigate the correct behavior. -- ___ Python tracker <https://bugs.python.org/issue35438> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35438] Extension modules using non-API functions
New submission from Eddie Elizondo : Three extension modules: _testcapimodule.c, posixmodule.c, and mathmodule.c are using `_PyObject_LookupSpecial` which is not API. These should instead use `PyObject_GetAttrString`, `PyType_GetSlot`. -- components: Library (Lib) messages: 331364 nosy: eelizondo priority: normal severity: normal status: open title: Extension modules using non-API functions type: behavior versions: Python 3.8 ___ Python tracker <https://bugs.python.org/issue35438> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35381] Heap-allocated posixmodule types
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +10089 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue35381> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35381] Heap-allocated posixmodule types
Change by Eddie Elizondo : -- title: Heap-allocated Posixmodule types -> Heap-allocated posixmodule types ___ Python tracker <https://bugs.python.org/issue35381> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35381] Heap-allocated Posixmodule types
Change by Eddie Elizondo : -- title: Heap-allocated Posixmodule -> Heap-allocated Posixmodule types ___ Python tracker <https://bugs.python.org/issue35381> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35381] Heap-allocated Posixmodule
New submission from Eddie Elizondo : After bpo34784, there are still two more cases of statically allocated types (DirEntryType & ScandirIteratorType). These should also be heap allocated to make posixmodule fully compatible with PEP384. -- components: Library (Lib) messages: 330906 nosy: eelizondo priority: normal severity: normal status: open title: Heap-allocated Posixmodule versions: Python 3.8 ___ Python tracker <https://bugs.python.org/issue35381> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue16086] tp_flags: Undefined behaviour with 32 bits long
Change by Eddie Elizondo : -- pull_requests: +10032 ___ Python tracker <https://bugs.python.org/issue16086> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34784] Heap-allocated StructSequences
Change by Eddie Elizondo : -- pull_requests: +9062 ___ Python tracker <https://bugs.python.org/issue34784> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34784] Heap-allocated StructSequences
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +8929 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue34784> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34784] Heap-allocated StructSequences
New submission from Eddie Elizondo : PyStructSequence_NewType does not currently work. Read the full analysis here: https://mail.python.org/pipermail/python-dev/2018-September/155069.html This aims to fix the implementation of PyStructSequence_NewType. -- components: Library (Lib) messages: 326205 nosy: eelizondo priority: normal severity: normal status: open title: Heap-allocated StructSequences versions: Python 3.6, Python 3.7, Python 3.8 ___ Python tracker <https://bugs.python.org/issue34784> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34522] PyTypeObject's tp_base initialization bug
Eddie Elizondo added the comment: @ronaldoussoren * This change currently works for all CPython. If you are using this pattern then you probably want to be using PyType_FromSpec rather than having a static PyTypeObject as discussed in PEP384: https://www.python.org/dev/peps/pep-0384. In general, extensions should all be using PyType_FromSpec rather than static PyTypeObjects. * As mentioned by Erik Bray in the e-mail thread, the usage of a static PyTypeObject with a PyVarObject_HEAD_INIT requires a compile-time constant. This causes problems cross-compatibility problems, especially with Windows. You can read more here: http://iguananaut.net/blog/programming/windows-data-import.html In general, this pattern is just pervasive. It causes cross-compatibility issues, it can cause users to forget calling PyType_Ready on a type and we are better off without it. Again, please read the mail thread. Everything that I'm saying here is all discussed there. -- ___ Python tracker <https://bugs.python.org/issue34522> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34522] PyTypeObject's tp_base initialization bug
Eddie Elizondo added the comment: @ronaldoussoren Please read the complete analysis from the mailing list: https://mail.python.org/pipermail/python-dev/2018-August/154946.html. The description here was just a rehash and I probably missed some context. Particularly, when I said: "PyTypeObject's ob_type should always be set by PyType_Ready" I was referring to the PyTypeObject's that are statically set in C code. Metatypes explicitly have to set the ob_type and that's already handled. In the current state of things, you have static PyTypeObjects that are being used before calling PyType_Ready due to this macro. This change just standardizes the header of static PyTypeObject throughout the entire codebase. -- ___ Python tracker <https://bugs.python.org/issue34522> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34533] Apply PEP384 to _csv module
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +8451 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue34533> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34533] Apply PEP384 to _csv module
New submission from Eddie Elizondo : This applies the heap type refactoring from PEP 384 to the _csv module. -- components: Extension Modules messages: 324268 nosy: eelizondo priority: normal severity: normal status: open title: Apply PEP384 to _csv module type: enhancement versions: Python 3.8 ___ Python tracker <https://bugs.python.org/issue34533> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34522] PyTypeObject's tp_base initialization bug
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +8432 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue34522> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue34522] PyTypeObject's tp_base initialization bug
New submission from Eddie Elizondo : >From the documentation, it says that PyType_Ready should be called on `ALL` >type objects to finish their initialization >(https://docs.python.org/3/c-api/type.html#c.PyType_Ready). This means that a >PyTypeObject's ob_type should always be set by PyType_Ready. It turns out that this is not actually followed by all the core types in CPython. This leads to the usage of types that were not initialized through PyType_Ready. This fix modifies PyVarObject_HEAD_INIT to default the type to NULL so that all objects have to be fully initialized through PyType_Ready. Plus: * It initializes all the objects that were not being initialized through PyType_Ready. * Modifies PyType_Ready to special case the ob_type initialization of PyType_Type and PyBaseObject_Type. * It modifies the edge case of _Py_FalseStruct and _Py_TrueStruct. Read more: https://mail.python.org/pipermail/python-dev/2018-August/154946.html -- components: Interpreter Core messages: 324195 nosy: eelizondo priority: normal severity: normal status: open title: PyTypeObject's tp_base initialization bug type: behavior versions: Python 3.8 ___ Python tracker <https://bugs.python.org/issue34522> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue33058] Enhance Python's Memory Instrumentation with COUNT_ALLOCS
Change by Eddie Elizondo : -- keywords: +patch pull_requests: +5852 stage: -> patch review ___ Python tracker <https://bugs.python.org/issue33058> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue33058] Enhance Python's Memory Instrumentation with COUNT_ALLOCS
Eddie Elizondo added the comment: @serhiy.storchaka tracemalloc can't distinguish between the usage of gc allocs, normal mallocs, and free list reuse. -- ___ Python tracker <https://bugs.python.org/issue33058> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue33058] Enhance Python's Memory Instrumentation with COUNT_ALLOCS
Eddie Elizondo added the comment: Currently, Python has very few instrumentation when it comes to the types of allocations that it's doing. For instance, we currently can't identify how many times an object uses free lists vs actual mallocs. Currently, there's a special build which can be used by compiling with "-DCOUNT_ALLOCS". However, this build is not ABI compatible with extensions. Meaning you have to recompile all the modules that are used. Doing this on a large scale scenario (1000+ modules) is not feasible. Thus, I propose the following enhancements: * Make COUNT_ALLOCS ABI compatible Then: * Expand the counters to not only track allocs and frees but also distinguish between: * GC Counts: Allocs/Frees coming from PyObject_GC_Malloc PyObject_GC_Del * Memory Counts: Allocs/Frees coming from PyMem_Malloc/PyObject_Malloc PyObject_Free (modulo GC objects). * Free Lists: "Allocs/Frees" coming from specific object's free_lists/caches. * Misc: Extra kinds of "Allocs/Frees" such as code's zombie frame. -- ___ Python tracker <https://bugs.python.org/issue33058> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue33058] Enhance Python's Memory Instrumentation with COUNT_ALLOCS
New submission from Eddie Elizondo : [WIP] -- title: [WIP] Enhance Python's Memory Instrumentation with COUNT_ALLCOS -> Enhance Python's Memory Instrumentation with COUNT_ALLOCS ___ Python tracker <https://bugs.python.org/issue33058> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue33058] Enhancing Python
Change by Eddie Elizondo : -- nosy: elizondo93 priority: normal severity: normal status: open title: Enhancing Python type: enhancement versions: Python 3.8 ___ Python tracker <https://bugs.python.org/issue33058> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue33058] [WIP] Enhance Python's Memory Instrumentation with COUNT_ALLCOS
Change by Eddie Elizondo : -- title: [WIP] Enhancing Python's COUNT_ALLOCS -> [WIP] Enhance Python's Memory Instrumentation with COUNT_ALLCOS ___ Python tracker <https://bugs.python.org/issue33058> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue33058] [WIP] Enhancing Python's COUNT_ALLOCS
Change by Eddie Elizondo : -- title: Enhancing Python -> [WIP] Enhancing Python's COUNT_ALLOCS ___ Python tracker <https://bugs.python.org/issue33058> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue32898] [BUILD] Using COUNT_ALLOCS breaks build
New submission from Eddie Elizondo : The following build crashed: mkdir debug && cd debug ../configure --with-pydebug make EXTRA_CFLAGS="-DCOUNT_ALLOCS" The bug was introduced here: https://github.com/python/cpython/commit/25420fe290b98171e6d30edf9350292c21ef700e Fix: 1) s/inter/interp/ 2) Declare PyTypeObject -- components: Build messages: 312504 nosy: elizondo93 priority: normal pull_requests: 5578 severity: normal status: open title: [BUILD] Using COUNT_ALLOCS breaks build type: compile error ___ Python tracker <https://bugs.python.org/issue32898> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
