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

Fix lua-style dictionary operator performance #68925

Closed
wants to merge 1 commit into from

Conversation

anvilfolk
Copy link
Contributor

@anvilfolk anvilfolk commented Nov 20, 2022

Fixes #68834.

Untitled

I'm having the GDScript analyzer replace an IdentifierNode with a LiteralNode so the behavior of dict.someIndex becomes the same as dict["someIndex"].

I don't believe I can do this in the parser, because the parser does not know whether dict is a dictionary or not. That is something the analyzer infers.

Unfortunately, this does require us to go into GDScriptParser's singly-linked list list and do a search for the IdentifierNode so we can remove it from there and replace it with the LiteralNode. For large scripts, this is not very efficient at all. If GDScriptParser::Node had a prev as well as a next, then it would become a doubly-linked list and be O(1) instead of O(n), where n is the number of parse nodes in a script.

I will likely add some more documentation in the future, but technically, this is ready for review.

Quick note: I really only added if (base_type.kind == GDScriptParser::DataType::BUILTIN && base_type.builtin_type == Variant::DICTIONARY && p_subscript->attribute->type == GDScriptParser::Node::IDENTIFIER) { ... }. The rest of the lines are counting as added lines but only because they got indented inside the else.

@anvilfolk anvilfolk requested a review from a team as a code owner November 20, 2022 16:59
@adamscott
Copy link
Member

I would create a converter inside gdscript_parser.{h,cpp} definition of IdentifierNode instead of handling the conversion inside an if. It would make the if/else more readable.

@anvilfolk
Copy link
Contributor Author

Decided to close since there's a nice refactor in #68086 which fixes this and makes things better to boot :)

@anvilfolk
Copy link
Contributor Author

Closed again since @rune-scape is investigating allowing StringName indexed dictionaries, which would make this not particularly desirable :) See discussion on #68834 for benchmarking & conclusions :)

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.

Lua-style Dictionary operations are slower
5 participants