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

Async task timings #6333

Merged
merged 17 commits into from
Dec 1, 2024
Merged

Async task timings #6333

merged 17 commits into from
Dec 1, 2024

Conversation

dktapps
Copy link
Member

@dktapps dktapps commented Apr 19, 2024

Introduction

This PR adds support for asynchronously collecting timings, and for collecting timings from AsyncWorkers for AsyncTasks.

The implemented API allows custom collect callbacks to be registered, which allows plugins to include custom thread timings in timings reports. This is necessary because other threads' timings records can't be directly accessed from the main thread.

Relevant issues

Closes #6166

Changes

API changes

  • TimingsHandler::printTimings() is now deprecated
  • Added getToggleCallbacks(), getReloadCallbacks() and getCollectCallbacks() to TimingsHandler

Behavioural changes

  • Timings reports now include AsyncTask timings.
  • Timings may now take slightly longer to generate as the report generator has to wait for other threads to respond.
  • Timings report version 3 is now used. This tells the timings viewer to use the ThreadId part of timer group names to namespace timer IDs (necessary since timer IDs could otherwise collide between different threads).

Backwards compatibility

Should be fully backwards compatible

Tests

I tested this PR by doing the following (tick all that apply):

  • Writing PHPUnit tests (commit these in the tests/phpunit folder)
  • Playtesting using a Minecraft client (provide screenshots or a video)
  • Writing a test plugin (provide the code and sample output)
  • Testing via command and pasting reports

Samples

This requires a version of the timings viewer including the following commit: pmmp/timings@13cefa6

An example report can be viewed here: https://timings.pmmp.io/?id=338211

dktapps added 6 commits April 19, 2024 18:05
this is a rough implementation and needs further work.
there's currently an issue where the first async worker to boot up doesn't enable timings if it was enabled by default in pocketmine.yml and I have no idea why - the task definitely gets scheduled so it doesn't make any sense.
we should probably add callbacks for collecting timings too, but that's a bit more complicated right now.
this allows custom threads to have timings collected via custom mechanisms, since we can't directly access another thread's timings.
for RakLib, for example, we'll need an IPC command, while for workers we need an AsyncTask.
@dktapps dktapps added Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP Category: UI Related to the user interface (e.g. commands, terminal output) labels Apr 19, 2024
@dktapps
Copy link
Member Author

dktapps commented Apr 19, 2024

There are some unrelated changes in the PR which I made to facilitate this feature, such as Promise::all() supporting 0 promises. These should be separated before merging.

@dktapps dktapps added the Multiple changes PR contains multiple changes and requires separation label Nov 10, 2024
dktapps added a commit that referenced this pull request Nov 13, 2024
not supporting this has caused problems every time this function has been used in reality so far (#6092 and #6333).
@dktapps dktapps requested a review from a team as a code owner November 13, 2024 14:59
github-actions[bot]
github-actions bot previously approved these changes Nov 13, 2024
@dktapps dktapps added Status: Blocked Depends on other changes which are yet to be completed and removed Multiple changes PR contains multiple changes and requires separation labels Nov 14, 2024
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Nov 16, 2024
dktapps added a commit to pmmp/Language that referenced this pull request Nov 16, 2024
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Nov 16, 2024
these are all caused by the same bug phpstan/phpstan#10924
@dktapps dktapps removed the Status: Blocked Depends on other changes which are yet to be completed label Nov 16, 2024
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Nov 19, 2024
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 1, 2024
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Dec 1, 2024
@dktapps
Copy link
Member Author

dktapps commented Dec 1, 2024

This is finished for now.

However, it would be worth making some changes to RakLib to enable some basic timings to be collected from it too. This would probably involve making Server->tick() and Server->receivePacket() protected to allow a PM class to extend and override them for timings encapsulation. A custom ServerEventSource to do timings would probably be a good idea too.

Currently RakLib's architecture (and the IPC system) are not flexible enough for timings integration, so they'll be skipped for this PR. This should be done as a follow-up.

If plugin devs use this system to report their own threads' timings, we shouldn't rely on them not to just pass a random int as the thread ID.
@dktapps dktapps merged commit 61560ec into minor-next Dec 1, 2024
30 checks passed
@dktapps dktapps deleted the async-task-timings branch December 1, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core Related to internal functionality Category: UI Related to the user interface (e.g. commands, terminal output) Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant