WIP: New Concurrent class #906
Conversation
I haven't been able to find the root cause of this issue yet, but it stems from pthreads' handling of copied static arrays. When pthreads copies a static array, modifying the array on the parent thread triggers heap corruption and other nasties for reasons I am yet to fully understand. The test case in the PR demonstrates the bug. It needs to be noted that: - the crash does not occur unless the `PTHREADS_INHERIT_CLASSES` flag is enabled (hence, it is to do with the way static arrays are being handled) - the crash occurs in GC when removing members of the array (`Test::$array = [];` works, also `unset(Test::$array[0]);`) - the crash does not occur if the static array is populated with scalar values or arrays, only objects.
…n-static thread local properties
…nnotated_thread_locals
|
I'm not sure I understand the deal with thread-locals. Why is it necessary to confine them to a new class? What if I want thread locals without the performance hit involved with mutability? The additional runtime checks appear to be very minor, so I don't see why they should be relegated to a separate class. I think it would be better if the statics fix could be handled separately from the |
| @@ -138,6 +145,10 @@ ZEND_END_MODULE_GLOBALS(pthreads) | |||
| #define PTHREADS_PG(ls, v) PTHREADS_FETCH_CTX(ls, core_globals_id, php_core_globals*, v) | |||
| #define PTHREADS_EG_ALL(ls) PTHREADS_FETCH_ALL(ls, executor_globals_id, zend_executor_globals*) | |||
|
|
|||
| #define PTHREADS_ACC_THREADLOCAL 0x10 | |||
There was a problem hiding this comment.
Is there any way this could be automatically assigned to an unused flag? I'm just wondering about possible future collisions with ZEND_ACC flags which could be difficult to debug.
There was a problem hiding this comment.
Yep, it's not unproblematic. But if this PoC works, I'll look for an as properties unused ZEND_ACC_* flag that we can reuse. Automatic assignment won't work :-/
| @@ -102,13 +103,19 @@ extern zend_class_entry *pthreads_socket_entry; | |||
| (Z_TYPE_P(o) == IS_OBJECT && instanceof_function(Z_OBJCE_P(o), pthreads_volatile_entry)) | |||
| #endif | |||
|
|
|||
| #ifndef IS_PTHREADS_CONCURRENT | |||
| #define IS_PTHREADS_CONCURRENT(o) \ | |||
| (Z_TYPE_P(o) == IS_OBJECT && instanceof_function(Z_OBJCE_P(o), pthreads_concurrent_entry)) | |||
There was a problem hiding this comment.
Perhaps it would be better to make Concurrent extends Volatile instead of adding boilerplate checks for rhis.
|
I'm not so sure I like introducing a new class for this. Is the performance overhead that much, since this is only done at copy time? I get that it introduces a little extra overhead, but given that it has the potential to save on some serialisation overhead (which is forcibly incurred on every instance property currently), the potential speed improvements of TLS could negate this. Maybe some benchmarks would help to see this more clearly. Also, the name "Concurrent" isn't particularly good, since it is pretty generic. It could refer to any concurrency primitive (thread, process, actor, communicating sequential process, green thread, etc). I'm glad to see an implementation of this, though :) |
Introduction of
Concurrentclass. This new mutable class features property visibility, a thread-local annotation(@thread_local),ArrayAccesscallback support and proper__get/__set/__isset/__unsethandling.Additionally this PR fixes #861, however with a BC break. Copying of static properties is confined to only default values and not assigned values.