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

Reduce CPU usage in the connection thread loop #213

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Conversation

vovukman
Copy link

@vovukman vovukman commented Jan 1, 2023

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
Copy link

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

@ErikKalkoken
Copy link

ErikKalkoken commented Jun 3, 2023

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
Copy link

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
Copy link
Contributor

bdraco commented Jan 6, 2024

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
Copy link
Contributor

bdraco commented Jan 6, 2024

I cleaned this up a bit more in #271

@bdraco
Copy link
Contributor

bdraco commented Jan 6, 2024

I took #213 (comment) into account in #271

@amyreese amyreese merged commit 49f577b into omnilib:main Feb 20, 2024
@amyreese
Copy link
Member

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

@bdraco
Copy link
Contributor

bdraco commented Feb 20, 2024

Thank you!

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.

7 participants