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

Script load times increased since dev28 #636

Closed
TrademarkTM opened this issue Jun 4, 2017 · 54 comments
Closed

Script load times increased since dev28 #636

TrademarkTM opened this issue Jun 4, 2017 · 54 comments
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.

Comments

@TrademarkTM
Copy link

TrademarkTM commented Jun 4, 2017

Hello again Bensku.
I've noticed a big problem in the new 28c version of Skript. My scripts are taking way more time to load.

28c and 29

[16:20:35] [Server thread/INFO]: loading 'ItemXP.sk'
[16:20:35] [Server thread/INFO]: loading trigger 'on load'
> 2 seconds here?
[16:20:37] [Server thread/INFO]: loading trigger 'on break'
[16:20:37] [Server thread/INFO]: loading trigger 'on rightclick holding a pickaxe'
[16:20:37] [Server thread/INFO]: loading trigger 'on place'
[16:20:37] [Server thread/INFO]: loading trigger 'on block damage'
[16:20:37] [Server thread/INFO]: loading trigger 'on command "gm"'
[16:20:38] [Server thread/INFO]: loaded 6 triggers and 0 commands from 'ItemXP.sk'

Took ~ 3 seconds to load.

27

[16:25:35] [Server thread/INFO]: loading 'ItemXP.sk'
[16:25:35] [Server thread/INFO]: loading 'ItemXP.sk'
[16:25:35] [Server thread/INFO]: loading trigger 'on load'
[16:25:35] [Server thread/INFO]: loading trigger 'on break'
[16:25:35] [Server thread/INFO]: loading trigger 'on rightclick holding a pickaxe'
[16:25:35] [Server thread/INFO]: loading trigger 'on place'
[16:25:35] [Server thread/INFO]: loading trigger 'on block damage'
[16:25:35] [Server thread/INFO]: loading trigger 'on command "gm"'
[16:25:35] [Server thread/INFO]: loaded 6 triggers and 0 commands from 'ItemXP.sk'

Less than 1 second to load.

My script is this one: https://hastebin.com/qehorawava.vbs
Hope you can fix it! Thanks!

@aescraftbr
Copy link

Kinda noticed that too.
In 27 it took like 13seconds to load all my skripts, now it takes 20+ seconds.

@netherstar
Copy link

Confirm, on dev 27 it took 6 seconds to load this very simple skript.

on join:
    wait 2 ticks
    make player execute command "/sw join"

@KorkugunuB
Copy link

I have the same problem

@Blueyescat
Copy link
Contributor

Takes 1 minute+ for my some skripts :)

@bensku
Copy link
Member

bensku commented Jun 5, 2017

Need to do some profiling to figure out what is causing this. I hope it is just some random mistake I made - if throwing vectors in causes it, I'll have interesting time trying to fix it.

@bensku
Copy link
Member

bensku commented Jun 6, 2017

@tuanjr For me it takes <1 second with dev25 and about same time with latest version. I need some test material, a script that:

  • Does not use any addons or functions from other scripts
  • Loads significantly slower with latest Skript, if possible like 10+ seconds slower
  • You will post there; someone else who is not me might want to test it too

To clarify: I'm not interested about, for now, any even potentially addon-related slowdowns with latest Skript. I will take look at them after I have done everything I can to get Skript itself running at least as fast or as slow as before.

From what I have seen now, SkriptParser's parse_i is hotspot and slow, but that is nothing new. It has been so for years...

@bensku bensku added the investigating The core developers are currently investigating this issue. Usually used for complex cases. label Jun 6, 2017
@Sashie
Copy link
Contributor

Sashie commented Jun 9, 2017

untitled
that load time indeed!!

@Sashie
Copy link
Contributor

Sashie commented Jun 9, 2017

you should add some custom timings to the parser to see how long it takes to load each effect/expression and a few timings to see whats happening in each syntax parse

@bensku
Copy link
Member

bensku commented Jun 9, 2017

I'll consider that once someone actually provides test scripts I asked for in previous post.

@TrademarkTM
Copy link
Author

TrademarkTM commented Jun 9, 2017

The first lines of code of my script are pure sk and are taking 2 seconds to load:

options:

	xp.maxlevel: 100
	
	xp.base.pickaxe: 200
	xp.increase.pickaxe: 0.2
	
	xp.blocks.pickaxe: stone-0.6;cobblestone-0.2;andesite-1;diorite-1;granite-1;diamond_ore-8;emerald_ore-100;lapis_lazuli_ore-10000
	
	xp.base.shovel: 200
	xp.increase.shovel: 0.15
	
	xp.blocks.shovel: dirt-0.3;sand-0.5;gravel-1.0;grass-0.4;red sand-0.6


on load:
	clear {config::*}
	set {config::base::pickaxe} to {@xp.base.pickaxe}
	set {config::increase::pickaxe} to {@xp.increase.pickaxe}
	loop split "{@xp.blocks.pickaxe}" at ";":
		set {config::blocks::pickaxe::%subtext of loop-value from characters 1 to first index of ""-"" in loop-value - 1 parsed as item%} to subtext of loop-value from characters first index of "-" in loop-value + 1 to length of loop-value
	set {config::base::shovel} to {@xp.base.pickaxe}
	set {config::increase::shovel} to {@xp.increase.pickaxe}
	loop split "{@xp.blocks.shovel}" at ";":
		set {config::blocks::shovel::%subtext of loop-value from characters 1 to first index of ""-"" in loop-value - 1 parsed as item%} to subtext of loop-value from characters first index of "-" in loop-value + 1 to length of loop-value

The full script was taking < 1s before.

@Snow-Pyon
Copy link

Snow-Pyon commented Jun 10, 2017

Can confirm, tested and it takes around 1.916 seconds with the dev28 whereas it's around 0.901 seconds with the dev25 and 0.6 seconds with the dev27.

@bensku
Copy link
Member

bensku commented Jun 10, 2017

Ok, this seems useful. I'll try to figure out what is wrong and how to fix it.

Also nice to know that dev27 actually improved performance by about 33% :)

@bensku
Copy link
Member

bensku commented Jun 11, 2017

I have disabled number to vector converter for next release to solve another issue. When it is out, please test if it helps with performance.

@Sashie
Copy link
Contributor

Sashie commented Jun 12, 2017

It certainly didn't help make my test go any faster :c (compiled from source)
One thing to note is that on multiple starts it would finish in a semi random amount of time between about 9m40s to 10m50s

@bensku
Copy link
Member

bensku commented Jun 13, 2017

I'm out of ideas. Will try something once I'm back home (in about week), but it might break stuff even more. Help would be appreciated...

@andrewjunggg
Copy link

dev25 took 4 seconds to reload my script,
dev29 will crash my server (timed out) every time i reload it.
and vector converter have an issue with RandomSk if i remembered well. Maybe you should check that too 😉

@Snow-Pyon
Copy link

@nutella25 he will not change the code just because an addon which isn't supported anymore is conflicting with it, you can just enable the soft api exceptions option on your config.sk and problem solved.

@Sam1370
Copy link

Sam1370 commented Jun 20, 2017

Someone change the title to "28c and 29 take way longer to load scripts."

@TheBentoBox TheBentoBox changed the title 28c takes way longer to load a script. Script load times increased since dev28 Jun 20, 2017
@Sam1370
Copy link

Sam1370 commented Jun 20, 2017

Nevermind, saying "increased" is better :P

@Sam1370
Copy link

Sam1370 commented Jun 20, 2017

Should I keep using dev29 or use a version that's older?

@TheBentoBox
Copy link
Member

If the load times aren't an issue for you there's no real reason to not use dev29. If you don't want to use dev28 or dev29 because of the load times though, I'd go back to dev25, since dev26-27 both had a number of issues, mainly with chat stuff due to the JSON chat support.

@Sam1370
Copy link

Sam1370 commented Jun 23, 2017

@TheBentoBox I'm using json.sk (https://www.spigotmc.org/resources/json-sk.8851/). Does that count as json chat support because I need that. Also the load times are a real issue for me, my skripts in total take 1 minute and 14 seconds to load as of my last one. So if I do /skript reload scripts it crashes the server, so instead I have to restart the server (which isnt a problem since no one really joins on it) but it's still a real hassle :(
EDIT: I used dev25 and it works great, skripts reload in 41 secs which I think isn't enough to make Spigot crash the server

@Tristag
Copy link

Tristag commented Jul 9, 2017

This is prob not the best suggestion, however, what if you just allow some features to be turned off/on through the config?

It seems they increased post-25 so basically one could turn off the features causing the load times while keeping ones that don't on, so it's at least something new :)

Anyways, good luck on this and it is immensely appreciated!

@Snow-Pyon
Copy link

Snow-Pyon commented Jul 9, 2017

This is prob not the best suggestion, however, what if you just allow some features to be turned off/on through the config?

Well, this is something that Mirreski wanted to do (look at the features.sk file) and I doubt it actually works but the system is still there so this could be possible.

@Tristag
Copy link

Tristag commented Jul 9, 2017

Maybe not my worst idea then lol

There are some bugs that I wish I could have fixed, however the exponential load times of the new versions make it impossible to run. I understand that it's really hard to fix all the bugs though so I won't complain, just my reasoning for the suggestion.

@bensku
Copy link
Member

bensku commented Jul 31, 2017

I have changed parser a bit, and will perform further changes either for next release or the release after that. While fixing a parser bug, I noticed that there are few relatively easy ways to improve parsing performance.

@Tristag
Copy link

Tristag commented Jul 31, 2017

That sounds awesome! Can't wait! Thanks a ton man!

@bloggy
Copy link

bloggy commented Sep 21, 2017

That's from July, still working on it?

@bensku
Copy link
Member

bensku commented Sep 21, 2017

I have still not received sufficient test case, which is a script which take 1+ minutes to parse with dev25, parses way slower with latest release and does not use addons. Do you have one, @bloggy?

Edit: I mean, I do have some test files, but they do not seem to parse much faster with dev25...

@bloggy
Copy link

bloggy commented Sep 21, 2017

I am sure that I have NO script that is pure skript without addons, but I wonder why u are unable to reproduce it? There must be something happened to the parser making it so slow?

EDIT:

But it isn't hard to make a skript that takes forever to reload with the latest skript version.
Here is an example (this is pure skript btw):
https://hastebin.com/tovajupamu.coffeescript
(This takes 30 seconds to reload in v2.2-dev31c)

Just copy / paste the same events and stuff in the same file over and over again. The more lines it has, the longer the parser takes to do its job.

Reloading the skript in my link even kicks me out of my server (time out / connection lost).

Btw. copying the lines of that script again and putting them at the end of the same file and doing a reload -> will crash my server because skript is freezing it then.

EDIT 2:

Reloading the same script as linked will take 9 seconds to reload on skript v2.2-dev25.

For me, having 58 scripts, Skript is unusable until you fix the parser loading time. :-(
Let me know if you need further information for helping with this.

@bensku
Copy link
Member

bensku commented Sep 21, 2017

30 seconds vs. 9 seconds is probably good enough. I can probably use your test script to figure out what is wrong, given enough time for debugging.

I guess that is what I will be doing next weekend. Wish me luck :)

@bensku bensku added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. and removed investigating The core developers are currently investigating this issue. Usually used for complex cases. labels Sep 21, 2017
@bensku bensku added this to the 2.3 milestone Sep 21, 2017
@bensku
Copy link
Member

bensku commented Sep 22, 2017

Done, closing when next release is out. Uhh, that took a long.

@TheBentoBox
Copy link
Member

The hero we need!

@bloggy
Copy link

bloggy commented Sep 22, 2017

Really? You fixed it so fast? :-) Mind sharing what the issue was? And when will you release the update? I really need it :-)

Thanks for your time and effort!

@TheBentoBox
Copy link
Member

@bloggy It appears to have been the location from vector expression judging by commits. Additionally, since this was added as a milestone for 2.3, it seems this fix will be released when 2.3 is, which will happen once the issues on the 2.3 milestone list are resolved.

@Snow-Pyon
Copy link

Snow-Pyon commented Sep 22, 2017

@bloggy you're still able to get the build which is supposed to fix this with the nightly builds repository:
https://github.com/bensku/skript-build

Be aware that these builds aren't stable at all.

@bensku
Copy link
Member

bensku commented Sep 22, 2017

Right now nightly builds are 100% broken due to other changes. Maybe tomorrow...

@bloggy
Copy link

bloggy commented Oct 10, 2017

Can you please tell us when you are planning on releasing the fixed version? Still unable to use Skript with 1.12 because of this bug. :-( Hope you can push an update for this.

@Syst3ms
Copy link
Contributor

Syst3ms commented Oct 10, 2017

He fixed it in recent commits, you can check out the nightly build repo (linked above) to try these versions

@TheBentoBox
Copy link
Member

TheBentoBox commented Oct 10, 2017

@bloggy dev25 works completely fine with 1.12.0

@bensku
Copy link
Member

bensku commented Oct 10, 2017

I do not have time to work on Skript right now, hopefully next weekend or so changes this.

@bloggy
Copy link

bloggy commented Oct 11, 2017

@TheBentoBox
for me it does not work 100% on 1.12.2 with your mentioned version:
https://hastebin.com/omujaciteq.vbs

Also tried the latest nightly build but then all of my skript commands stop working (it always says: unknown command). But does not throw any errors.

@TheBentoBox
Copy link
Member

@bloggy Are you on 1.12.0 or a newer one? (1.12.1/1.12.2)

@bloggy
Copy link

bloggy commented Oct 11, 2017

I am always on the latest paper version (which is 1.12.2)

@TheBentoBox
Copy link
Member

TheBentoBox commented Oct 11, 2017

That's the issue, which is why I specified 1.12.0 in my comment above. You can use dev25 on 1.12.0 just fine without load time issues.

@bensku bensku closed this as completed Oct 27, 2017
@bensku
Copy link
Member

bensku commented Oct 27, 2017

@bloggy New release is out. And indeed, nightlies had broken commands - I fixed it today.

@Sam1370
Copy link

Sam1370 commented Nov 3, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

No branches or pull requests