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

Perf: Remove unnecessary deepcopy calls in Schemaview.induced_slot #296

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

sneakers-the-rat
Copy link
Contributor

Following up on linkml/linkml#1604

This is one of the largest sources of slow performance in the non-graph-based generators.

Currently, when I run the test suite, it takes me 50 minutes, which is entirely too long imo - we want tested contributions, but if i have to wait 50 minutes before submitting a PR or between every change then the odds start to really climb that i'm just going to forget about it.

This gets us down to 40 minutes essentially for free (and when using pydanticgen makes a ~60s generation process into ~2-3s).

First: we can remove the deepcopy at the end - that's wholly unnecessary, since induced_slot is already deepcopied earlier in the method. we don't need to protect something that will die immediately when the method ends.

Second, we also don't need the deepcopy in the middle of the method:

  • get_slot is cached.
  • it draws from all_slots which is also cached.
  • all_slots makes a (shallow) copy of self._get_dict as well
  • if get_slot doesn't find anything in all_slots it makes a shallow copy of the class attribute

so we shouldn't need deepcopy in induced_slot since the thing we draw from should be cached and copied already - future calls to get_slot should get the cached copy even if we mutate it downstream. I replaced it with a regular copy since that's much faster if it's necessary at all.

Third: Avoid double-calling get_slot with differing call signature since the first call is only necessary if we take the first leg of the if cls statement.

Fourth: the output of induced_slot is also cached, but it still seems like we're getting cache misses from the difference between slot name as a str vs SlotDefinitionName. not fixed in this PR.

Cumulative time in Schemaview.induced_slot before: 510s (95832 calls)
Cumulative time in Schemaview.induced_slot after: 243s

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2e35728) 62.90% compared to head (fe8e821) 62.90%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #296   +/-   ##
=======================================
  Coverage   62.90%   62.90%           
=======================================
  Files          62       62           
  Lines        8529     8529           
  Branches     2239     2239           
=======================================
  Hits         5365     5365           
  Misses       2554     2554           
  Partials      610      610           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

3 participants