unix-ffi/re: Fix the PCRE2 memory leaks. - #1153
Conversation
Every call to search() allocated a match data block with pcre2_match_data_create_from_pattern() and never freed it again, leaking a few kilobytes per call, on the no-match path as well. Free it once the offsets have been copied out of it. The module level functions compile a pattern that the caller never gets to see, and that was leaked as well. Free it when the call is done; the match object that is returned does not refer to it. Note that a pattern returned by re.compile() still has to be kept alive by the caller and cannot be released automatically, because MicroPython does not run __del__ on instances of Python classes. Signed-off-by: Kirill Lukonin (Evil Wireless Man) <klukonin@gmail.com>
The error code and the error offset were passed as bytes(4). Such objects are immutable, and the error offset is a PCRE2_SIZE, which is 8 bytes on a 64-bit target, so a failing compile wrote 4 bytes past the end of the buffer. Use writable arrays of the right size instead, and report the values in the assertion. Signed-off-by: Kirill Lukonin (Evil Wireless Man) <klukonin@gmail.com>
The test measures the resident set size around a few thousand calls and fails if it keeps growing. It covers every entry point that makes PCRE2 allocate: matching with a compiled pattern, the module level functions, and compiling itself, including a pattern that fails to compile. Without the preceding fixes it reports between 4.6 and 17.8 kilobytes of growth per call, depending on the entry point. Signed-off-by: Kirill Lukonin (Evil Wireless Man) <klukonin@gmail.com>
Every call to re.search(), and to the functions next to it, compiled the pattern it was given. Keep the compiled patterns in a small cache instead, the way CPython does, so that using the same pattern again does not compile it a second time. compile() returns the cached pattern as well, so re.compile(p) is re.compile(p), as it is in CPython. Matching against a repeated pattern gets about twice as fast, compiling one about eight times. Because MicroPython cannot release a compiled pattern by itself, the cache also decides what is kept: a cached pattern stays for the lifetime of the program, and a pattern that this module compiled for its own use is freed again afterwards. The cache owns what it holds and never evicts it. A pattern that is still in use, by the caller or by a call further up the stack, must not be freed underneath it, which a replacement callback passed to sub() can otherwise trigger. The cache is bounded instead: once it is full, further patterns are compiled and, where this module owns them, freed again after use. Signed-off-by: Kirill Lukonin (Evil Wireless Man) <klukonin@gmail.com>
| try: | ||
| return r.search(string) | ||
| finally: | ||
| if owned: |
There was a problem hiding this comment.
If you put the owned check inside _free then it will probably be smaller once byte-compiled, so it won't have to be repeated multiple times.
| def _compile(pattern, flags): | ||
| # These are output arguments and must be writable and of the size that | ||
| # pcre2_compile() writes: int for the error code, PCRE2_SIZE for the offset. | ||
| errcode = array.array("i", [0]) |
There was a problem hiding this comment.
This is actually the same as doing bytes(4). Check mp_binary_get_size in py/binary.c.
TL;DR: maybe the smallest fix is to update erroffset size from 4 to 8 and use int.from_bytes to perform the bytes->int conversion. Probably speed is the least of your worries for this function.
An array constructor takes 11 bytes (ignoring the extra entry in the string pool for the format string as that's already brought in elsewhere):
11:02 LOAD_NAME array
14:02 LOAD_METHOD array
10:03 LOAD_CONST_STRING i
80 LOAD_CONST_SMALL_INT 0
2b:01 BUILD_LIST 1
36:02 CALL_METHOD 2
whilst creating a bytes object just 5:
11:03 LOAD_NAME bytes
84 LOAD_CONST_SMALL_INT 4
34:01 CALL_FUNCTION 1
int.from_bytes should probably suffice in this case to do the conversion, as this is only for printing (guaranteed?) positive numbers with a fixed upper size.
Shortening the message may also help here: how about "compile error %d at %d"?
| self.key = None # set while this pattern is held by the cache | ||
|
|
||
| def _free(self): | ||
| # MicroPython does not run __del__ on instances of Python classes, so |
There was a problem hiding this comment.
You may want to see if weakref can help here, added in 1.27.0.
There was a problem hiding this comment.
That would be great to use weakref... except it's not enabled on the unix port by default.
If using weakref would significantly simplify this PR then maybe we can consider enabling weakref by default on the unix port.
There was a problem hiding this comment.
Does it require too much movements or quite possible to enable for the next major unix-port version?
There was a problem hiding this comment.
If I'm not mistaken it should be a matter of applying this diff, and once you've installed the packages mentioned in docs/develop/gettingstarted.rst [1], make -C ports/unix should yield an interpreter called ports/unix/build-standard/micropython with weakref enabled.
diff --git i/ports/unix/mpconfigport.h w/ports/unix/mpconfigport.h
index 9b66d8684..f8b4cda5b 100644
--- i/ports/unix/mpconfigport.h
+++ w/ports/unix/mpconfigport.h
@@ -125,6 +125,8 @@ typedef long mp_off_t;
#define MICROPY_TRACKED_ALLOC (1)
#endif
+#define MICROPY_PY_WEAKREF (1)
+
// VFS stat functions should return time values relative to 1970/1/1
#define MICROPY_EPOCH_IS_1970 (1)[1] build-essential, libffi-dev, git, and pkg-config for Debian/Ubuntu.
There was a problem hiding this comment.
It's definitely possible to enable weakref for the next release.
There was a problem hiding this comment.
I pointed at the beginning of the PR
There was a problem hiding this comment.
Well, this looks definitely neater. Thanks for updating the code!
The error code and the error offset were turned into array.array() objects on the assumption that a bytes object cannot be written to. ffi passes the buffer of either one straight to the C function, so the array buys nothing and only makes the constructor larger at every call. Use bytes again, and read the values back out of them with int.from_bytes(). What was wrong is the size of the error offset. It is a PCRE2_SIZE, which is 8 bytes on a 64-bit target, so a failing compile wrote past the end of the 4 bytes it was given. Size it from PCRE2_SIZE_SIZE, which is already derived from uctypes.ULONG for this purpose. Signed-off-by: Kirill Lukonin (Evil Wireless Man) <klukonin@gmail.com>
A compiled pattern was released by an explicit _free(), so the module had to keep track of who owned each one: the cache marked the patterns it held, the module level functions freed the ones that did not fit into it, and compile() freed nothing at all because that pattern belongs to the caller. The cache could not evict either, because a pattern that is still in use, by the caller or by a replacement callback further up the stack, must not be freed underneath it. weakref.finalize() does all of this instead: a pattern is freed once nothing refers to it any more. The ownership tracking goes away, the module level functions go back to the two lines they were before, and the cache becomes a plain optimisation that evicts the way CPython does. A pattern returned by re.compile() no longer has to be kept alive by the caller to avoid leaking it. This needs a build with MICROPY_PY_WEAKREF enabled, which the unix port does not do by default yet. Signed-off-by: Kirill Lukonin (Evil Wireless Man) <klukonin@gmail.com>
4 participants
!!! This needs a build with
MICROPY_PY_WEAKREFenabled !!!unix-ffi/renever frees anything it gets from PCRE2. Every match leaks thematch data block, and every compiled pattern leaks as well, so a program or module (such as json) that
uses re module in a loop grows without bound.
Measured on a
ports/unixbuild (1.30.0-preview) against libpcre2-8, residentset size around 2x5000 calls:
Pattern.search(), match / no matchPattern.match()/sub()/split()/findall()re.search()/re.match()re.sub()/re.split()/re.findall()re.compile(), same pattern / distinct patterns70k operations grew the process from 4 MB to 367 MB before.
With the fixes it is flat now.