[issue9787] Release the TLS lock during allocations
Kristján Valur Jónsson krist...@ccpgames.com added the comment: Closing this since it applies only to our custom tls implementation. Most platforms use native tls now. -- resolution: - wont fix status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Kristján Valur Jónsson krist...@ccpgames.com added the comment: Making this low priority since it applies only to platforms without Windows and pthread support. -- priority: normal - low versions: +Python 3.3 -Python 3.2 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Kristján Valur Jónsson krist...@ccpgames.com added the comment: I'll rework this for python 3.x and see where that gets us. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Kristján Valur Jónsson krist...@ccpgames.com added the comment: New patch, based on the cpython tip. -- Added file: http://bugs.python.org/file24959/tlspatch.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Kristján Valur Jónsson krist...@ccpgames.com added the comment: hm, for some reason this patch isn't viewable in side-by-side -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Martin v. Löwis mar...@v.loewis.de added the comment: You may find this hard to believe, but we do in fact embed python into other applications. This is actually very easy to believe. appMalloc, is in this case, the canonical memory allocator in UnrealEngine. But it could be any other memory allocator so that is beside the point. This seems to be the core of the issue. Any other memory allocator would *not* inquire about the state of Python. Any other memory allocator would not even be aware that it is used by Python. What appMalloc is doing, in this case, is for every allocation, to get the python TLS pointer. There is nothing wrong with this I find this wrong. It violates the software layering. The memory management layer is not supposed to access upper layers (such as the interpreter state, or the application state). Now, regardless of the above, surely it is an improvement in general if we make tighter use of the TLS lock. It's not a good idea to hold this lock across malloc calls if we can avoid it. The patch is harmless, might even be an improvement, so why object to it? The code change itself is harmless, yes. The comment is not. It imposes a requirement on Python (namely, that the malloc implementation may be free to make calls to Python) which is harmful. The malloc implementation just has no business looking at the thread state. So I remain -1 on this change. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Kristján Valur Jónsson krist...@ccpgames.com added the comment: The malloc implementation just has no business looking at the thread state. Of course it does, if it you want to have any hope of instrumenting your python memory usage with detailed python runtime information. Your statement islike saying: A profiler has no business looking at the thread callstack. Note that we are not making any new requirements on python here. Merely facilitating the process, for those implementations that _wish_ to do so (at their own risk.) So, although you have nothing against the patch as such, you are against it on the principle that I am using it to facilitate something that you disapprove of. I find that a quite unreasonable position. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Martin v. Löwis mar...@v.loewis.de added the comment: Note that we are not making any new requirements on python here. But you are. So far, there was no guarantee whatsoever about the state of Python when malloc is called. You are now introducing a requirement that Python must be in a certain state to make it correct to call malloc. IOW, this innocent change actually introduces a new feature. So, although you have nothing against the patch as such I think it's harmless - I don't think it is a good patch. It shouldn't matter whether or not it is applied (i.e. there is no change to Python that I can see). you are against it on the principle that I am using it to facilitate something that you disapprove of. I find that a quite unreasonable position. No. It's not the usage that I disapprove but, but the new requirement on Python. If you were able to do your profiling in a manner compatible with (the current) Python, it would be certainly fine with me. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Martin v. Löwis mar...@v.loewis.de added the comment: What is appMalloc, and what does it have to do with some Python lock? You seem to suggest that some malloc implementations make use of Python interpreter internals. I would call that a bug in the malloc implementation (it violates standard layering assumptions), and so I'm -1 on inclusion of this patch. -- nosy: +loewis ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Kristján Valur Jónsson krist...@ccpgames.com added the comment: You may find this hard to believe, but we do in fact embed python into other applications. In this case, it is UnrealEngine, to drive a complex, console based game. Yes, embedding python is much harder than it need be and I'll submit some patches to make that easier someday, but that's not the point of this. appMalloc, is in this case, the canonical memory allocator in UnrealEngine. But it could be any other memory allocator so that is beside the point. The problem at hand, however, is this memory allocator _may_ have to inquire about the state of Python. It would do this, for example, to gather statistics about Python's memory use. This is critically important when developing console based applications, where every Kilobyte counts. Embedding python sometimes requires the replacement of the standard libc malloc with something else. What appMalloc is doing, in this case, is for every allocation, to get the python TLS pointer. There is nothing wrong with this, this is a GIL free operation, and it will return either NULL or the current TLS. And it works, except, when appMalloc (through python's malloc) is being invoked from the TLS entry creation mechanism itself. Then it deadlocks. Now, regardless of the above, surely it is an improvement in general if we make tighter use of the TLS lock. It's not a good idea to hold this lock across malloc calls if we can avoid it. The patch is harmless, might even be an improvement, so why object to it? It removes yet another gotcha that embedders, or those replacing malloc, (or instrumenting python's malloc use) have to face. Cheers, K -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Kristján Valur Jónsson krist...@ccpgames.com added the comment: I forgot to add: The API that our (instrumented) malloc implementation is calling is: PyGILState_GetThisThreadState(); -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
New submission from Kristján Valur Jónsson krist...@ccpgames.com: Holding the keymutex lock during malloc and free operations is not a good idea. The reason is, that custom implementations of malloc and free, can use the TLS themselves. This is, for example, true in embedded situations, where one wants to replace malloc with, e.g. appMalloc, (to monitor the memory useage of Python) and appMalloc itself uses python TLS to find the current python State. This change makes the malloc and free calls outside the lock. The change in PyThread_set_key_value, requiring an extra lock allocate, has no significant performance impact since this is a rare api. -- components: Interpreter Core files: tlspatch.patch keywords: patch, patch messages: 115743 nosy: krisvale priority: normal severity: normal status: open title: Release the TLS lock during allocations type: behavior versions: Python 3.2 Added file: http://bugs.python.org/file18782/tlspatch.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Antoine Pitrou pit...@free.fr added the comment: You have a bug in PyThread_delete_key_value() (to_free = NULL?). Also, you should move the /* NB This does *not* free p-value! */ comments at the right places. -- nosy: +pitrou ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue9787] Release the TLS lock during allocations
Kristján Valur Jónsson krist...@ccpgames.com added the comment: You're right. Added a new version of the patch. -- Added file: http://bugs.python.org/file18793/tlspatch.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue9787 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
