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

Open
wants to merge 6 commits into
base: minor-next
Choose a base branch
from
Open

Async task timings #6333

wants to merge 6 commits into from

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(), getResetCallbacks() 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

Follow-up

Requires translations:

Name Value in eng.ini
N/A Compiling timings report

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=330357

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.

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.

None yet

1 participant