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

Fix performance issue in RingUtils::checkFused #5410

Merged
merged 1 commit into from
Jul 8, 2022
Merged

Fix performance issue in RingUtils::checkFused #5410

merged 1 commit into from
Jul 8, 2022

Conversation

rachelnwalker
Copy link
Contributor

We noticed a performance hit for sanitizing very ringy structures (specifically for chained bucky balls and a smaller performance hit for nanotubes) after the 2022_03_02 release and traced it back to #4722. I did some profiling and found that copying all of the ring neighbors in RingUtils::checkFused seemed to be causing the performance degradation.

Before this change:

Time to sanitize...
Chained bucky balls (128 atoms): 0.184651 s
Nanotube (2412 atoms): 0.361771 s

After this change:


Time to sanitize...
Chained bucky balls (128 atoms): 0.025208 s
Nanotube (2412 atoms): 0.277536 s

d-b-w
d-b-w approved these changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@d-b-w d-b-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice.

It looks like checkFused() could be const in ringNeighs while you're here.

greglandrum
greglandrum approved these changes Jul 8, 2022
View reviewed changes
Copy link
Member

@greglandrum greglandrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.
It's great when they are this simple to fix. :-)

@greglandrum greglandrum added bug Cleanup Code cleanup and refactoring labels Jul 8, 2022
@greglandrum greglandrum added this to the 2022_03_5 milestone Jul 8, 2022
@greglandrum greglandrum merged commit 30547e4 into rdkit:master Jul 8, 2022
@rachelnwalker rachelnwalker deleted the sanitization_performance_issue branch July 8, 2022 17:04
greglandrum pushed a commit that referenced this pull request Aug 4, 2022
@rachelnwalker @greglandrum
Fix performance issue in RingUtils::checkFused that slowed down sanit…
9279d42 
…ization (#5410)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greglandrum greglandrum greglandrum approved these changes

@d-b-w d-b-w d-b-w approved these changes

Assignees
No one assigned
Labels
bug Cleanup Code cleanup and refactoring
Projects
None yet
Milestone
2022_03_5
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rachelnwalker @greglandrum @d-b-w