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

Improvements to dumpstats performance #660

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

aaronjeline
Copy link
Collaborator

Changes and re-organizes the Root Cause Analysis to aid in readability and pefromance

@aaronjeline
Copy link
Collaborator Author

One question here is "Is the root cause analysis code enough to move to a separate file?"

@kyleheadley
Copy link
Member

This looks good as far as my understanding goes. Code it clean with comments, algorithms are reasonable and I assume it works and meets your goals. I vote yes for a separate file, but it's not a big deal.

I'm sorry I wasn't able to take part in the discussion earlier, but if this is critical performance and set operations are holding it back, I can suggest not using them. You can add data directly on the graph nodes (or rebuild the graph entirely for this purpose), compute a spanning tree (nearly linear with modern algorithms) or whatever is necessary for cycle elimination, and then collect data in one pass. If the cycles are handled well you may not even need to de-dup, and can use append-only arrays. Was this considered? How does current performance scale?

Even if the above works, this update is fine to include in the mean time.

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

Successfully merging this pull request may close these issues.

2 participants