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

Performance optimizations for imports of sqlalchemy_utils #357

Open
mfulgo opened this issue Jul 30, 2024 · 7 comments · May be fixed by #356
Open

Performance optimizations for imports of sqlalchemy_utils #357

mfulgo opened this issue Jul 30, 2024 · 7 comments · May be fixed by #356

Comments

@mfulgo
Copy link
Contributor

mfulgo commented Jul 30, 2024

I'm seeing a notable amount of time during application initialization spent loading continuum, and more specifically, packages of sqlalchemy_utils that aren't needed.
image

I believe that with some more-focused imports, this could be improved.

mfulgo added a commit to mfulgo/sqlalchemy-continuum that referenced this issue Jul 30, 2024
Importing functions from slqalchemy_utils directly causes the entire
library to get imported, even though many of the library's modules go
unused. Using more-focused imports should improve initialization time in
performance-sensitive situations.

This commit also removes a few unused imports.

Closes kvesteri#357
@marksteward
Copy link
Collaborator

Do you have a flame graph for after? My long-term goal is to get rid of sqlalchemy_utils as a dependency.

@mfulgo
Copy link
Contributor Author

mfulgo commented Jul 31, 2024

Unfortunately, not on hand: I'd need to vendor the patched dependency and do some profiling. Is that something you'll want before merging the PR?

@mfulgo
Copy link
Contributor Author

mfulgo commented Aug 7, 2024

@marksteward - I did vendor this with the patch. Unfortunately, I missed that python will still load the __init__.py in the root package. So, I'll update update the PR to copy in the parts of sqlalchemy_utils it needs and remove the dependency.

@marksteward
Copy link
Collaborator

Thanks, I had a feeling that would be the case! This will need a major release because of breaking changes, but I think we can take the opportunity to simplify some things:

  • JSONType should probably use JSON
  • ImproperlyConfigured could become ImportError, or it could just call import flask again.
  • get_primary_keys doesn't need to be wrapped in OrderedDict any more, and could probably be inlined in the places it's currently used. This could probably use inspect(table).primary_key, and maybe we make this sqla 2.0+ only. Likewise identity.
  • get_column_key should be inlined and maybe rewritten.

I appreciate this is a bunch of work, so happy to pick up some of these/add tests/have a chat, depending on your preference.

@mfulgo
Copy link
Contributor Author

mfulgo commented Aug 8, 2024

Since there will be breaking changes anyway...

  1. Do you want to drop support for python 3.7, which is EOL? How about 3.8, which goes EOL in October?
  2. Can the __init.py__ in the plugins directory be empty, instead of having it import every plugin? That should simplify the flask plugin imports a bit: If you import the flask plugin, and flask isn't available, you'll get an import error immediately. Plus, it will avoid the init overhead of loading plugins that one may not need.

For my PR, I'm planning to keep the code and functionality as close to what exists as possible. I haven't migrated from sa 1.4 yet, so my vendored version of this needs to keep support. (And I want to avoid the possibility of introducing new bugs by changing up those implementations.)

@mfulgo
Copy link
Contributor Author

mfulgo commented Aug 8, 2024

After applying the changes from #356 to my vendored copy, initialization time for dropped to about 36ms. (I then ripped out the postgres native_versioning and plugins code that we're not using to get it down to ~13ms.)

@marksteward
Copy link
Collaborator

Sorry for not replying, this is fantastic. I'll take a look at landing it this weekend.

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 a pull request may close this issue.

2 participants