py/objtype: Implement __del__ data model method for user classes. #18005
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18005 +/- ##
=======================================
Coverage 98.38% 98.39%
=======================================
Files 171 171
Lines 22296 22314 +18
=======================================
+ Hits 21937 21955 +18
Misses 359 359 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note that the first 2 commits here are common with #18004; debugging this feature was the main driver for finding those cleanups. |
|
Code size report: |
|
Remaining TODO:
|
Cleans up 2423493, and fixes builds that include LFS but not MICROPY_ENABLE_FINALISER. Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
|
Other than for freeing resources obtained through FFI, what use case would it have in MicroPython? |
What use cases do finalisers have in MicroPython at all? Anything you could want to do with them in C, you could have a perfectly valid reason to want to do with python. The original impetus for this feature was related to @agatti's current work implementing I've got another library I've been developing that would benefit from this, that's for managing I2C devices --- with efficient transaction queueing, caching, and automatic fusion of adjacent transactions. When a sqlite returns a handle to a stored procedure it's memoized, if you're using a python-level ORM object to manage that procedure, it ought to be able to set a More broadly, this applies to any sorts of code that uses ECS/SoA data layout, i.e. with user-defined type with a non-pointer value indexing some kind of table; or when doing any kind of database that needs to track reach-ability across non-trivial foreign-key relationships. In many cases, context managers are the "clean code approved" way to manage external lifetimes --- but if every problem was one with lifetime boundaries that were that simple, we'd already all just be writing Rust instead of Python. Plus, to implement a context manager that's robust against bugs like e.g. #2621, that too also needs to make use of I'm currently working on an implementation of the |
The calls signature for gc_malloc was changed in 5ed578e, without cleaning up existing code on the rationale that the previous bool is automatically converted to an int with the same meaning. This commit goes back and cleans up existing invocations to make their behavior more readable in a modern context. Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
There was a problem hiding this comment.
f7bc55c
This subtle tweak to the memory layout is the only distinction between a failing and passing version of this exact test, on otherwise exactly the same commits.
|
|
|
Ok, so this original But, why isn't it the case that first block of the allocation is marked as // mark first block as used head
ATB_FREE_TO_HEAD(area, start_block); |
|
|
This check is a remnant of the original finaliser implementation from 4f7e9f5, all the way from pre-1.0. This was a valid optimisation back then, but after its ordering with other finaliser code changed in 8a2ff2c the invariant it exploits is no longer valid, and it breaks down completely in the case of variable-length allocations made with `mp_obj_malloc_var_with_finaliser`. This resulted in a latent bug in the VFS system, where depending on the precise memory layout, VFS finaliser routines could sometimes be inadvertently missed. This bug was discovered when encountering an identical issue with an attempt to implement `__del__` for user classes in micropython#18005. As can be seen from the test runs with the instrumentation in micropython#18013, this branch is also never actually hit by normal code outside of VFS. Therefore, whatever the original intent and whether or not it was ever, it's not actually an optimisation now; removing this condition is a strict improvement to garbage collector code size and performance. Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
This check is a remnant of the original finaliser implementation from 4f7e9f5, all the way from pre-1.0. This was a valid optimisation back then, but after its ordering with other finaliser code changed in 8a2ff2c the invariant it exploits is no longer valid, and it breaks down completely in the case of variable-length allocations made with `mp_obj_malloc_var_with_finaliser`. This resulted in a latent bug in the VFS system, where depending on the precise memory layout, VFS finaliser routines could sometimes be inadvertently missed. This bug was discovered when encountering an identical issue with an attempt to implement `__del__` for user classes in micropython#18005. This perhaps should be removed completely for the reasons discussed in micropython#18014, but removing it appears to induce some other issue with the tls module; this version just expands the check in the interest of avoiding that. Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
|
After much tribulation, I've determined that the bug I thought I was tracing is actually something else entirely. Not finalising Why, then, does GC think that object (I guess there's also two other competing hypotheses: |
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
|
I've benchmarked this PR against the benchmarks in #18019 Results from Pico2, Cortex M33 @ 300 MHzThe comparisons I think are most instructive here are these:
Note that the cost of adding a |
6829e35 to
f21dc52
Compare
|
@dpgeorge @mattytrentini thoughts on this? |
|
I tried adding an example of del for user C modules in #13011 and ran into issues I never resolved with the lack of predictability on when they would actually be run; often requiring multiple collect's and /or run "extra code" then collect again to get any sembalance of reliability. From you investigations above you're already looking a lot deeper into the gc than I ever got but perhaps some of the comments in my old PR support your findings. |
Thanks for the reference! I was kinda able to observe this happening with some of the objects in the VFS LFS module, but that code has its own complexities that muddy the waters --- it's super useful having a MCVE of this issue using just the C api. Probably gonna have to buckle down and write some proper instrumentation code here soon to visualize the heap structure and what pointers it actually is, keeping these objects alive here... |
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
Summary
This PR implements the required functionality to allow the garbage collector to run user-defined
__del__methods as finalisers.Testing
I've added full-coverage tests of this feature that I've tested for sensitivity across both the Unix port and my RP2040 and RP2350 boards. All tests pass on my RP2 boards, and with one very difficult exception noted below all tests also pass on unix.
tests/basics/class_del_postadded.pyonly passes on the unix builds I've built withDEBUG=1; but fails on myDEBUG=0builds. I suspect that this is an underlying garbage-collector bug, rather than a bug with this implementation of__del__specifically. So far I've failed to discover exactly why it is this occurs, though I've confirmed using the debugger on a non-debug non-stripped build that everything up to and including the call tomp_obj_malloc_var_with_finaliserdoes run as expected.Trade-offs and Alternatives
This PR claims two additional bits from the
mp_obj_type_t.flagsfield, forMP_TYPE_FLAG_IS_INSTANCEDandMP_TYPE_FLAG_HAS_FINALISER. The latter bit is required in order to track whether instances should be allocated using the_with_finaliserallocators, while the former is required to track whether or it's safe to allow the__del__method to be inserted into a class after-the-fact. It's possible thatMP_TYPE_FLAG_IS_INSTANCEDcould be eliminated by forbidding this type of insertion entirely.Alternatively, almost all of the code complexity in this PR could be eliminated by unconditionally allocating objects from user-defined classes with
mp_obj_malloc_var_with_finaliser; however the performance penalty that this would introduce with all of the failed attribute lookups on non-using classes, leads me to favor this more-complex approach.