Fix several greenlet leaks #261
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ess. Add a timeout to all such instances.
Thread-local C++ objects should be a portable way to get thread destructors. The slp_* functions need to be compiled as C because we call them from assembly. forward declarations should also be c linkage, otherwise MSVC complains.
This eliminates the complexity of green_updatecurrent, I believe. Much more can be simplified as well. Still need to add the thread cleanup functions.
Currently, we're somewhat slower than the global variable case, as expected. | Benchmark | gl-1.1.2 | tl1 | |------------------------------|:--------:|:---------------------:| | create a greenlet | 68.7 ns | 88.0 ns: 1.28x slower | | switch between two greenlets | 301 ns | 435 ns: 1.45x slower | | getcurrent single thread | 17.9 ns | 31.8 ns: 1.78x slower | | chain(100000) | 256 ms | 275 ms: 1.08x slower | | Geometric mean | (ref) | 1.37x slower | Also some Windows debugging.
Amusingly, this only caused issues on 64-bit Python 3.9 and 3.10 an Windows.
…global in g_switchstack into a single copy, thus messing up the stack. Try accessing it with the global variable each time. This makes clang, at least, look it up each time, even with optimizations on.
We're now using Py_AddPendingCall to clean up some thread-local things that don't clean themselves up. I'm still working to be sure we break cycles.
You need special syntax to return a pointer in a noinline function in MSVC. Because the parser ignores, without warning, __declspec after a * character. https://docs.microsoft.com/en-us/cpp/cpp/declspec?view=msvc-160
Improve the debugging to not do that. Also limit it to a suspected issue for now.
This should let us restore support for older compilers.
I haven't tested this on other platforms or with a more complex app like gevent yet though.
…s we take. Also add a testing API to be sure cleanups have finished.
Unfortunately, one still does and we have to manually cleanup a few others. Those can be fixed later.
This is less error prone.
This was referenced Oct 12, 2021
|
I admire your dedication @jamadden. This was hard work! And as always also great documentation work. |
|
This still may not fix all leaks. In 1.1.2, the test run was leaking 26 greenlets; now its down to leaking 16. So there's improvement, but still work to do. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most commonly when threads are involved. This fixes #252 (and probably gevent/gevent#1795) without any user code changes. (There are at least 6 tests that leak greenlets in 1.1.2, including older tests not specifically written to leak; no tests leak greenlets in this branch.)
This moves to thread-local variables for most things instead of module-global variables that have to be constantly checked to be sure they're valid. This requires a compiler that supports that; all tested compilers/platforms do, but it may end up dropping support for some very old platforms. (If that winds up mattering, it can be worked around at a speed cost similar to the way Python 2.7 on Windows is working.)
The core of the fix is to eliminate an uncollectable reference cycle/chain: greenlets no longer reference their thread-state dictionary directly. This results in an API and ABI difference for C subclasses of greenlets, so this will need to be version 2.0. I'd like to take advantage of that to address some other things for 2.0 as well. (Part of the fix is
O(n)when a thread that uses greenlets dies, so for environments where there are many GCable objects, and many threads rapidly starting and ending, but a small memory leak is acceptable, there should be a way to disable this part of the fix — plus provide some visibility into it so we know whether it really is a bottleneck.)I've built and tested gevent against this branch with no issues.
In the single-threaded case, this is very slightly slower than 1.1.2; once some more code cleanups are done I will see if we can get back some of that performance.