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 alias loading #7084

Merged
merged 18 commits into from
Nov 23, 2024
Merged

Async alias loading #7084

merged 18 commits into from
Nov 23, 2024

Conversation

Efnilite
Copy link
Member

@Efnilite Efnilite commented Sep 15, 2024

Description

Makes alias loading async to avoid the server freezing for ~6 seconds on load.
Surprisingly, changing it to loading async so far has not caused any issues.

Todo before merging

  • Test with addons (will do later as I can't download SkBee right now)
  • Test with example scripts

Target Minecraft Versions: any
Requirements: none
Related Issues: none

@Efnilite Efnilite added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 15, 2024
@GStudiosX2
Copy link

GStudiosX2 commented Sep 15, 2024

So far has not caused any issues:

Breaks tests
Edit: actually TBF I don't know if it's related or not

@Asleeepp
Copy link
Contributor

So far has not caused any issues:

Breaks tests Edit: actually TBF I don't know if it's related or not

they seem to be related to aliases, so its related

@sovdeeth
Copy link
Member

So far has not caused any issues:

Breaks tests Edit: actually TBF I don't know if it's related or not

their local machine was able to load them prior to tests running, hence the local tests passed. The github machines aren't as quick.

@Efnilite
Copy link
Member Author

So far has not caused any issues:

Breaks tests Edit: actually TBF I don't know if it's related or not

blame Microsoft for poor infrastructure

@Efnilite Efnilite marked this pull request as ready for review September 16, 2024 15:12
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@ShaneBeee
Copy link
Contributor

Call me crazy, but this isn't async.

...
[22:10:31 INFO]: [SkBee]  - 15 sections
[22:10:31 INFO]: [SkBee] Checking for update...
[22:10:32 INFO]: [SkBee] Plugin is up to date!
[22:10:32 INFO]: [SkBee] Successfully enabled v3.6.1 in 1.78 seconds
[22:10:32 INFO]: [spark] Starting background profiler...
[22:10:32 INFO]: Done preparing level "world" (5.865s)
[22:10:32 INFO]: Running delayed init tasks
-->>> ( Take note on the 4 second gap here) <<---
[22:10:36 INFO]: [Skript] Loaded 233074 aliases in 7945ms
[22:10:36 INFO]: [Skript] Loading variables...
[22:10:36 INFO]: [Skript] Loaded 0 variables in 0.0 seconds
[22:10:36 INFO]: [Skript] All scripts loaded without errors.
[22:10:36 INFO]: [Skript] Loaded 1 script with a total of 1 structure in 0.08 seconds
[22:10:36 INFO]: [Skript] Finished loading.
[22:10:36 INFO]: Done (18.045s)! For help, type "help"
[22:10:36 WARN]: Can't keep up! Is the server overloaded? Running 3631ms or 72 ticks behind
[22:10:36 INFO]: Timings Reset
> 

This appears to just delay Skript loading aliases/variables/scripts til later, causing the server to freeze after the "Running delayed init tasks"
Im no master with Async but I feel like this could potentially cause issues down the road.
This also doesn't appear to create any benefit.

In my opinion (I repeat... my opinion), I think rather than trying to worry about async, and "speed", should worry about cleaning up the 233074 aliases.

@Efnilite
Copy link
Member Author

Call me crazy, but this isn't async.

...
[22:10:31 INFO]: [SkBee]  - 15 sections
[22:10:31 INFO]: [SkBee] Checking for update...
[22:10:32 INFO]: [SkBee] Plugin is up to date!
[22:10:32 INFO]: [SkBee] Successfully enabled v3.6.1 in 1.78 seconds
[22:10:32 INFO]: [spark] Starting background profiler...
[22:10:32 INFO]: Done preparing level "world" (5.865s)
[22:10:32 INFO]: Running delayed init tasks
-->>> ( Take note on the 4 second gap here) <<---
[22:10:36 INFO]: [Skript] Loaded 233074 aliases in 7945ms
[22:10:36 INFO]: [Skript] Loading variables...
[22:10:36 INFO]: [Skript] Loaded 0 variables in 0.0 seconds
[22:10:36 INFO]: [Skript] All scripts loaded without errors.
[22:10:36 INFO]: [Skript] Loaded 1 script with a total of 1 structure in 0.08 seconds
[22:10:36 INFO]: [Skript] Finished loading.
[22:10:36 INFO]: Done (18.045s)! For help, type "help"
[22:10:36 WARN]: Can't keep up! Is the server overloaded? Running 3631ms or 72 ticks behind
[22:10:36 INFO]: Timings Reset
> 

This appears to just delay Skript loading aliases/variables/scripts til later, causing the server to freeze after the "Running delayed init tasks" Im no master with Async but I feel like this could potentially cause issues down the road. This also doesn't appear to create any benefit.

In my opinion (I repeat... my opinion), I think rather than trying to worry about async, and "speed", should worry about cleaning up the 233074 aliases.

well yeah before the scripts get parsed the aliases need to be loaded. it loads the aliases async so a seperate thread can do all the work, which does result in a ~50% speed boost. i could make it parse the scripts off the main thread as well but idk how feasible that is

Skript.warning(Skript.m_no_scripts.toString());
reloaded(sender, logHandler, timingLogHandler, "config, aliases and scripts");
});
Aliases.loadAsync().thenRun(() -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thenRun will resume back on the main thread, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@Efnilite Efnilite added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Nov 20, 2024
@sovdeeth sovdeeth merged commit 75d0701 into SkriptLang:dev/feature Nov 23, 2024
5 checks passed
@Efnilite Efnilite deleted the async-aliases branch November 23, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants