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

[ide] collect_implementations in statistics.ml needs optimization #9504

Closed
nadako opened this issue May 28, 2020 · 6 comments
Closed

[ide] collect_implementations in statistics.ml needs optimization #9504

nadako opened this issue May 28, 2020 · 6 comments
Assignees

Comments

@nadako
Copy link
Member

nadako commented May 28, 2020

18:49
nadako well.. if I'm understanding correctly, that loops through like 90% of the codebase for every field, because there are things like IEventDispatcher (edited)

@nadako
Copy link
Member Author

nadako commented May 28, 2020

This one is pretty high-priority for us, because "find references" basically hangs in processing and I think it affects a lot of code.

nadako added a commit that referenced this issue May 28, 2020
did while trying to figure out #9504
@nadako
Copy link
Member Author

nadako commented May 28, 2020

so far we've found out that cl_descendants that this function is working on is a mess, containing duplicates and (apparently) cycles which causes an infinite loop.

nadako added a commit that referenced this issue May 28, 2020
this can happen a lot with interfaces involved, also this workarounds messy cl_dependants situation (see #9504 (comment))
@nadako
Copy link
Member Author

nadako commented May 28, 2020

ok, I couldn't spot actual cycles, so my new version is that it's just hundreds of duplicates of the very basic framework classes (e.g. Sprite), which has hundreds of duplicates of application base classes, which has hundreds of duplicates of concrete classes and so on, which leaves us with a huge amount of data to process so maybe it would finish if I gave it a day or so ^^

#9506 sidesteps this issue, but we still need to figure out why there are duplicates in cl_descendants and fix it (also because it might affect DCE and other places in the code that uses it).

@nadako
Copy link
Member Author

nadako commented May 28, 2020

More observations:

Adding assert (not (List.memq descendant c.cl_descendants)) to add_descendant doesn't break compilation (cache server or not), so I assume everything is fine on that front, however "Find References" now gives this error:

[Error - 23:57:23] Request textDocument/references failed.
  Message: Field _createBottomButtons should be declared with 'override' since it is inherited from superclass de.innogames.strategycity.shared.ui.window.BaseTabWindow

Which is something I've actually seen before when checking some other issues, so I guess we also have a pokemon catch somewhere that swallows the assertion failure?

(_createBottomButtons is the method that we're asking and it has override)

@nadako
Copy link
Member Author

nadako commented May 28, 2020

I think I know where duplicates are coming from. We're calling find_references (and thus collect_statistics which sets up cl_descendants) multiple times here:

acc @ (find_references tctx com with_definition name pos kind)

@nadako
Copy link
Member Author

nadako commented May 28, 2020

However, if I move "set up descendants" out and call it before the find_references loop, it still gets duplicates, and I don't know why.

Simn pushed a commit that referenced this issue May 29, 2020
* minor collect_implementations cleanup

did while trying to figure out #9504

* don't process already processed classes in collect_implementations

this can happen a lot with interfaces involved, also this workarounds messy cl_dependants situation (see #9504 (comment))
@RealyUniqueName RealyUniqueName added this to the Hotfix milestone Jun 1, 2020
@RealyUniqueName RealyUniqueName self-assigned this Jun 1, 2020
RealyUniqueName added a commit that referenced this issue Jun 18, 2020
* minor collect_implementations cleanup

did while trying to figure out #9504

* don't process already processed classes in collect_implementations

this can happen a lot with interfaces involved, also this workarounds messy cl_dependants situation (see #9504 (comment))
RealyUniqueName added a commit that referenced this issue Jun 18, 2020
for any amount of symbols in an inheritance tree (closes #9504)
@RealyUniqueName RealyUniqueName modified the milestones: Hotfix, Bugs Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants