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

[3.x] Speedup running very big GDScript files #53507

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

qarmin
Copy link
Contributor

@qarmin qarmin commented Oct 7, 2021

Fixes #53506

It just changes List to Vector which is a lot of faster in iterating elements in really big files.

With file from above issue(100000 lines), time to run script via terminal dropped from 10 minutes to 5 seconds.

With small files I don't expect any performance improvements or degradation.

Not sure if this not broke other things.

@lawnjelly
Copy link
Member

lawnjelly commented Oct 7, 2021

This looks great 👍 , I'll try and have a look through and work out how the parser etc works as I'd also been meaning to profile gdscript and see if there was any low hanging fruit. (this will need a gdscript specialist to review primarily but I'll do what I can).

From what I can see of the use of statements, there shouldn't be any side effects.

The only slower operation than the original is the couple of push_fronts that are replaced, and these are likely hugely outweighed by the [] access speeds. I'll just have a look in an example project to see how often these are called, but I suspect it should not be an issue.

Yes the push_front seems to rarely get called, in jetpaca for example I don't think it is called at all, maybe it only occurs with a certain match statement in gdscript. So any penalties from that are likely to be small unless you had a file full of match statements I guess.

Copy link
Member

@lawnjelly lawnjelly 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 as far as I can see, but probably needs a look over by a gdscript specialist as well.

@Xrayez
Copy link
Contributor

Xrayez commented Oct 7, 2021

Yes the push_front seems to rarely get called, in jetpaca for example I don't think it is called at all, maybe it only occurs with a certain match statement in gdscript. So any penalties from that are likely to be small unless you had a file full of match statements I guess.

Yet another reason to use elif blocks over match, because according to my past profiling (in 3.2), using elif turned out to be faster.

But it's not like GDScript priority is performance I guess, it's dynamically-typed language after all. 🦆

@@ -1188,8 +1188,8 @@ static bool _guess_identifier_type(GDScriptCompletionContext &p_context, const S
}
}

for (const List<GDScriptParser::Node *>::Element *E = blk->statements.front(); E; E = E->next()) {
const GDScriptParser::Node *expr = E->get();
for (int z = 0; z < blk->statements.size(); z++) {
Copy link
Contributor

@AaronRecord AaronRecord Oct 7, 2021

Choose a reason for hiding this comment

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

Can't this use a range iterator? #50284
Or is that worse for performance in some way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Range iterators are implemented only in master branch, but I use 3.x branch in this commit

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

LGTM. The master branch already uses Vector for the block statements.

@akien-mga akien-mga added this to the 3.4 milestone Oct 8, 2021
@akien-mga akien-mga merged commit 07094f5 into godotengine:3.x Oct 8, 2021
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

6 participants