Performance optimizations for imports of sqlalchemy_utils

kvesteri / sqlalchemy-continuum

Versioning extension for SQLAlchemy.

BSD 3-Clause "New" or "Revised" License

575 stars 127 forks source link

Performance optimizations for imports of sqlalchemy_utils #357

Open mfulgo opened 1 month ago

mfulgo commented 1 month ago

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.

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

marksteward commented 1 month ago

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

mfulgo commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

Since there will be breaking changes anyway...

Do you want to drop support for python 3.7, which is EOL? How about 3.8, which goes EOL in October?
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 commented 1 month ago

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.)