py/gc: Remove finaliser check for AT_HEAD block type. #18014
Conversation
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>
|
Code size report: |
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>
|
Ok, after some detailed investigation it seems like it's actually that the GC has determined that these blocks are spuriously reachable for some other reason, which I'll need to dig harder to figure out. |
|
This does seem like a valid optimisation... the finaliser bit should only be set on GC blocks that are head blocks. |
That's what I thought for a very long time too --- but actually, at the specific point in time where this finaliser loop gets invoked, this is how the garbage collector indicates to the finaliser code which objects it's planning to collect! Anything with |
Ah, you're right! Otherwise it would finalise everything. Maybe it's worth adding a comment to describe the situation? |
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 what I believe to be 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 #18005.As can be seen from the test runs with the instrumentation in #18013, this branch is also not actually hit in most normal code --- it's not actually an optimisation now; removing this condition ought to be a strict improvement to garbage collector code size and performance.