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.

Sign up for GitHub

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

Jump to bottom

Performance got worse in the last 2 weeks #1127

Closed
ghaith opened this issue Mar 6, 2024 · 2 comments · Fixed by #1175
Closed

Performance got worse in the last 2 weeks #1127

ghaith opened this issue Mar 6, 2024 · 2 comments · Fixed by #1175
Labels
bug Something isn't working high-priority performance

Comments

@ghaith
Copy link
Collaborator

ghaith commented Mar 6, 2024

It might be the change with the parser errors, but I noticed that on big projects a lot of time would pass before we see the first error. We should investigate that.
@ghaith ghaith added bug Something isn't working performance labels Mar 6, 2024
@volsa
Copy link
Member

volsa commented Mar 11, 2024 •
edited

Did some quick research and it looks like I have unlocked an achievement, unintentionally fucking up the performance by a lot due to 94a74fd. Specifically the find_local_member method internally calls find_enum_variant_in_pou which works on raw (sub)string comparisons. I knew it would have some performance penalties (especially because there's no caching for already performed queries) but I didn't think it would be this bad. Anyways, seems like we need to refactor how enums are stored in the index hence closely related to #1112.

Picture of plc check ... profile, previously taking around 4 seconds on an internal project but now takes between 2 and 4 minutes
Screenshot 2024-03-11 at 10 42 59

@volsa
Copy link
Member

volsa commented Mar 11, 2024 •
edited

Also https://plc-lang.github.io/metrics/ wasn't able to detect any performance regressions, we really need to find some big open source library which we can include in the metrics...

@volsa volsa mentioned this issue Mar 26, 2024
Merged
@volsa volsa linked a pull request Mar 26, 2024 that will close this issue
refactor: Enum Representation in Index #1175
Merged
volsa added a commit that referenced this issue Apr 4, 2024
@volsa
refactor: Enum Representation in Index (#1175)
6a5304e 
This PR refactors how enum variants are stored in the index, removing the `enum_qualified_variables` entry in the Index and instead storing all enums variants in the [`DataTypeInformation`](https://github.com/PLC-lang/rusty/blob/b357a05cb064eb21fed110b6d7c5f543f72c18e8/src/typesystem.rs#L361) struct.

Querying enum variants was the main motivation here, as enums are stored unfavorably in the Index currently. Specifically enum variants are stored in an HashMap with their keys being their qualified name, which makes it hard to get all variants of an enum. For example an enum defined in main such as `Color : (red, green, blue)` is currently represented as `__main_color.red`, `__main_color.green` and `__main_color.blue` which requires raw string manipulation & comparison degrading performance as described in this [issue](#1127).

Retrieving all variants of an enum in this PR is done by querying for an enum retrieving its datatype, then accessing the `variants` field which is done in O(1) because we're querying the `type_index` which is behind a HashMap. Finding a specific enum variant is still linear however.

tl;dr The performance of `plc check` in one of our bigger internal projects is back to 2 seconds (from previously 2-4 minutes)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working high-priority performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: Enum Representation in Index PLC-lang/rusty
2 participants
@ghaith @volsa