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

Pre-compute node indices to greatly improve performance. #40

Merged
merged 2 commits into from
Feb 20, 2023

Conversation

estasney
Copy link

@estasney estasney commented Jan 31, 2023

Hello,

Thanks for publishing this add-on. When I first tried it, I thought it had crashed as the UI became unresponsive for 5-10 minutes.

Looking at the code, I noticed a similar pattern:

([node.id_ for node in sorted_nodes]).index(some_vertex)

By itself, this isn't too expensive a computation. However, it was being recreated several times in each for loop. On my not too complex model, this was computed 250,000+ times.

Since we only really need to do this once, creating a dictionary of:

{node.id_: index}

ahead of time, allows the export to run < 1 second.

I tested the output jbeam file with md5sum and the outputs were equivalent.

Note - This assumes that node.id_ is unique. I.e this will return different values if they are not unique:

# Index Method
node = 1
sorted_nodes = [1, 1, 3, 1]
>>> sorted_nodes.index(node)
>>> 0

# Dictionary Method
node = 1
sorted_node_idx = {x: i for i, node in enumerate(sorted_nodes)}
# {1: 3, 3: 2}
>>> sorted_node_idx[node]
>>> 3

Anecdotal testing showed export times reduced from ~2 minutes to ~1 second.
@estasney estasney marked this pull request as ready for review January 31, 2023 04:15
@50thomatoes50 50thomatoes50 merged commit 8eef2c9 into 50thomatoes50:master Feb 20, 2023
@50thomatoes50
Copy link
Owner

Thank you very much for the fix.

@estasney estasney deleted the speed-up branch February 20, 2023 17:25
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