Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop support for class/function/constant copying #64

Open
dktapps opened this issue Sep 27, 2021 · 1 comment
Open

Drop support for class/function/constant copying #64

dktapps opened this issue Sep 27, 2021 · 1 comment

Comments

@dktapps
Copy link
Member

dktapps commented Sep 27, 2021

A very large amount of pthreads' code is dedicated solely to copying code from the origin thread.

Right now, class copying takes place on 3 occasions:

  1. When a thread starts, all classes, functions and constants loaded on the parent thread (depending on PTHREADS_INHERIT_* flags, default is ALL) are copied to the child thread.
  2. When a thread starts, the thread's own class is copied from the parent (if it wasn't already). This always happens, regardless of the INHERIT flags used.
  3. When a task is run on a Worker, the task's class is copied from the parent. This always happens, regardless of the INHERIT flags used.
  4. When a Threaded object assigned by thread1 is read by thread2, the Threaded object's class is copied from thread1 to thread2. This always happens, regardless of the INHERIT flags used.

Reasons to stop doing this:

  • It's extremely costly (adding significant delays to thread start)
  • It can't make clean or complete copies. Defaults for stuff like static properties are unavailable after those properties are modified, and may contain data types that can't be easily copied (e.g. objects, arrays, resources). (no longer applicable to PHP 8.1)
  • It's a massive maintenance burden, accounting for the vast majority of compatibility issues that arise on any new PHP version.
  • It wastes a ton of memory - the copied classes might not be needed by the target thread anyway.
  • PocketMine-MP makes little to no usage of this code, preferring to rely on autoloading for better reliability, predictability, performance and memory usage (especially, opcache keeps memory usage low with cached code, if autoloading is used).
  • Most projects use Composer for autoloading, in which case it's preferable to have something like Introduced Thread::setAutoloadFile() #23
  • It worsens stability - there's so many things that can and do go wrong due to small undocumented changes in php-src.

Reasons to keep it:

  • Anonymous Threaded classes can't be supported without it, since they can't be autoloaded. Dropping code copying means we would no longer be able to have anonymous Threaded classes. I don't consider this to be a big enough reason to keep the gigantic maintenance burden of class copying around.

Obstacles

  • Point 2 above is a problem, since there's currently no other way for the Thread class to get into the actual thread right now other than having it copied from the parent. This could be solved by implementing something like Introduced Thread::setAutoloadFile() #23 .

Compatibility

Since PocketMine-MP doesn't currently use code inheritance at all (PTHREADS_INHERIT_NONE or PTHREADS_INHERIT_INI is typically used), it currently relies almost entirely on the autoloader anyway, so this change would have basically zero impact on compatibility.

@dktapps
Copy link
Member Author

dktapps commented Oct 2, 2021

It turns out there's another reason this is a problem: it becomes impossible to use threads within the scope of a single file (script), since the classes declared in the script won't be autoloadable without re-running the code that led to the creation of the thread in the first place.

ext-parallel might suck for a number of reasons, but one thing it really got right is the closure-based execution units.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant