Repository navigation
_PyStaticUnicode_Dealloc should not exist. #96458
Description
Activity
Seems this was introduced in gh-32032. Maybe @kumaraditya303 understands why it is needed or how it could be avoided? It's only use seems to be
deepfreeze.c. See also #91240, originally reported by @jkloth."Dealloc" in the name is certainly misleading. "Fini" would be more appropriate (and follow convention).
As to the function being unnecessary, it looks like you are right.
PyUnicode_AsUTF8AndSize()(and derivatives likePyUnicode_AsUTF8()) populates that "utf8" field, but only for non-ascii strings.
If we were statically initializing any non-ascii strings, we'd initialize the "utf8" and "utf8_length" fields, so we still wouldn't need_PyStaticUnicode_Dealloc().That said, would it be worth keeping a safeguard somewhere, to ensure we don't later add non-ascii strings where "utf8" isn't statically initialized?So is your proposal to just add the
compact->utf8initialization to deepfreeze.c, for non-ascii strings?Re: safeguard, I'm not sure why we'd need that, given that this is all generated code. Nobody in their right mind statically initializes unicode objects (the deepfreeze.py script has no mind :-).
So is your proposal to just add the compact->utf8 initialization to deepfreeze.c, for non-ascii strings?
I'm agreeing with Mark that we can eliminate
_PyStaticUnicode_Dealloc().Re: safeguard, I'm not sure why we'd need that
Yeah, keeping a safeguard doesn't seem that important. I'll strike that part of my earlier comment to reduce the noise.
I'm agreeing with Mark that we can eliminate
_PyStaticUnicode_Dealloc().How? Just delete the function and its calls? There was a reason it was put in (see #91240), related to leak detection tools.
tl;dr
_PyStaticUnicode_Dealloc()didn't actually need to do anything with theutf8field, and the PEP 623 PR dropped the other thing we were cleaning up. So_PyStaticUnicode_Dealloc()doesn't actually do anything any more.
There are a couple changes to consider here:
- when
_PyStaticUnicode_Dealloc()was added (gh-32032), it clearedPyASCIIObject.wstrand, for non-ascii strings,PyCompactUnicodeObject.utf8 - a few months later,
PyASCIIObject.wstrwas removed by PEP 623, in gh-92537 (leaving_PyStaticUnicode_Dealloc()to only (maybe) clearPyCompactUnicodeObject.utf8)
When I tried building main on Windows with the
_PyStaticUnicode_Dealloc()line commented out in deepfreeze.py, it showed 0 leaks. When I did the same with the original change (1), it showed the reported leak. Doing the same against the PEP 623 change (2) did not show that leak.This implies a number of possibilities:
_PyStaticUnicode_Dealloc()isn't needed any more
a. it only needed to clearwstr(which no longer exists)
b. something else changed- there was a non-ascii string getting frozen that was removed between (1) and (2)
- something else happened
- I did something wrong
I'm confident it is (1a). We are currently freezing 6 non-ascii strings. (Look for
.ascii = 0in deepfreeze.c.) So I would have seen a leak when I commented out_PyStaticUnicode_Dealloc().Thus we should be okay to drop
_PyStaticUnicode_Dealloc().- when
As long as
test_embedstill passes on Windows and Linux with_PyStaticUnicode_Dealloc()removed, all is good.Reacted by Eric Snow and Jeremy Kloth1a sounds good. Let’s delete this function. Who will make the PR?
1a will leak memory. I have tried it on Linux.
#96481 fixes this by statically initalizing utf8 representation.
1a will leak memory. I have tried it on Linux.
How are you checking? I did the following (after commenting out the
_PyStaticUnicode_Dealloc()line in deepfreeze.py):$ make -j8 ... $ ./python -X showrefcount -I -c pass [0 refs, 0 blocks]#96481 fixes this by statically initalizing utf8 representation.
+1
Since
utf8this is a cached value that's only set when needed, you'll have to execute some code that actually needs the utf8 encoding of one of the 6 non-ascii strings in deepfreeze.c. Or run the full leak discovery test suite.Reacted by Eric Snow- added a commit that references this issue
on Sep 3, 2022
Static data doesn't need to be freed. In fact, in cannot be freed.
It appears that the
utffield is dynamically allocated on static strings. Since this field is only needed for non-ascii strings, it should be static as well.Then
_PyStaticUnicode_Dealloccan be deleted.