-
Notifications
You must be signed in to change notification settings - Fork 226
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
Avoid O(N^2)
scan through globals during name resolution
#5001
Labels
enhancement
New feature or request
Comments
5 tasks
jfecher
changed the title
Avoid
Avoid May 8, 2024
O(N^2)
scan through genericsO(N^2)
scan through globals during name resolution
This code still exists in the elaborator here |
@TomAFrench we should be able to remove that check I believe. As long as globals are present in the def_maps. |
5 tasks
github-merge-queue bot
pushed a commit
that referenced
this issue
Oct 11, 2024
# Description ## Problem\* Resolves #5001 ## Summary\* Removes the search for a global by local module ID and name when elaborating a global variable declaration. Instead simply looks up the global by the ID that we have in the definition we are elaborating. In fact the elaboration starts from a global ID of an unresolved global. I could be way off the mark on whether this is the right thing to do. ## Additional Context I found a couple of things odd: * `NodeInterner::push_global` pushes a `DefinitionKind::Global(id)` with the exact ID that the `GlobalInfo` will have, ie. it is self-referential (just an observation). This is called during the collection of unresolved globals. * `Elaborator::add_global_variable_decl` only looks for globals by `local_id` and `name`, but _not_ by `definition`, even though at this point we know that the `definition` must contain a `GlobalId`, otherwise we would have stayed in `Elaborator::add_variable_decl`. * if a global cannot be found, it calls `NodeInterner::push_definition` with the `definition` in the parameters; since we know it contains a `GlobalId` and that the same ID must have been inserted in `push_global` as well, it will now be part of two definitions at different locations - not sure exactly what this means or looks like in code I added some assertions in the commits to check that during the tests on CI, the `global_id` and the `definition`, and any results found by the lookup by local+name, are always consistent, which is why in the end I thought that if the only thing we can find is the one we can get by ID, why not get just get it? ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem
the following code
in the resolver scans through all previous global's whenever a global is added, resulting in
O(num_globals^2)
:Happy Case
Store global variables in a set or (hash)map to allow looking them up quickly
Project Impact
Nice-to-have
Impact Context
No response
Workaround
None
Workaround Description
No response
Additional Context
No response
Would you like to submit a PR for this Issue?
None
Support Needs
No response
The text was updated successfully, but these errors were encountered: