omnilib / aiosqlite

asyncio bridge to the standard sqlite3 module
https://aiosqlite.omnilib.dev
MIT License
1.17k stars 93 forks source link

Reduce CPU usage in the connection thread loop #213

Closed vovukman closed 6 months ago

vovukman commented 1 year ago

Hi guys! I guess we can improve CPU usage by replacing periodical checking in the connection thread loop. See commit.

I'm not sure this is correct. But the commit passed all tests. What do you think about it?

vincebusam commented 1 year ago

This looks great to me. Without this patch, aiosqlite was the biggest CPU suck on my system.

ErikKalkoken commented 1 year ago

I also think it would be better to use the existing queue to send a stop signal instead of having the runner poll every 100ms. Then you also do not need the _running variable anymore, it's only function is signaling the runner to stop.

What I would do differently is, I would wrap the queue objects in a named tuple. Then it becomes a message, which can also have a stop_signal as property in addition to the future and the callable. Would be more readable and cleaner in my opinion then having it sometimes return a tuple and sometimes a None.

Something like this:

class Message(NamedTuple):
    """A queue message."""

    future: Optional[asyncio.Future]
    func: Optional[Callable]
    is_stop_signal: bool = False
MorningLightMountain713 commented 1 year ago

Can this get merged as is please.

Obviously, having a Message() class is a good idea. However, SQLAlchemy is using the internal queue (it shouldn't) so the structure on the queue internals can't change otherwise it breaks the dialect.

bdraco commented 8 months ago

Got here because I noticed a significant cpu drain from the queue with lots of threading locks. I think the queue could be replaced with SimpleQueue along with the changes in this PR.

bdraco commented 8 months ago

I cleaned this up a bit more in https://github.com/omnilib/aiosqlite/pull/271

bdraco commented 8 months ago

I took https://github.com/omnilib/aiosqlite/pull/213#issuecomment-1670856398 into account in #271

amyreese commented 6 months ago

Merged as part of #271. Thank you everyone for your patience!

bdraco commented 6 months ago

Thank you!