-
Notifications
You must be signed in to change notification settings - Fork 212
Unix Socket Implementation #822
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
Conversation
It looks good. I haven't tested it, and I don't know enough to fully understand it without reading up on things. But the code looks clean. One thing I noticed is that you inlined the tests in timer.py rather than putting them in an external file. Is that a temporary thing? I assume the |
oh yes absolutely the tests should be in an external file, those lines were more helping out with my prototyping vs being real tests. |
- Remove the self-referencing `./setup.py --version` in __init__.py.
- Remove WebSocketServer and related modules - Use Gio.SocketService implementation for the new unix sockets. - Use the new framer module to handle message framing and serializing over unix stream sockets. - Make more use of GLib signal/slot functionality for decoupling. - Drop the now unnecessary extra thread for Gtk.main()
1c0962c
to
07c3e70
Compare
- Make sure the right Levenshtein module is installed - Drop the unnecessary Framer.__str__ override.
return "" | ||
|
||
|
||
__data_directory__ = f'{Path(__file__).resolve().parent.parent}/data' | ||
__is_dev__ = True | ||
__version__ = _exec_get_("git", "describe", "--tags") or _exec_get_("./setup.py", "--version") or "DEV" | ||
__version__ = _exec_get_("git", "describe", "--tags") or "DEV" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git describe --tags
assumes the version is a git repo. So if it's not it will be "DEV"
This means for example if someone downloads the tarball source and build it will load "DEV" as the version. Maybe that's fair though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the important bit though is that ./setup.py --version
looks at ulauncher.__init__.__version__
which tries to run ./setup.py --version
which looks at ulauncher/__init__.__version__
.... and so on until your memory is exhausted ;)
In my case git describe --tags
didn't work because I use work trees, and the dev container fails because it can only see the worktree and not the full tree. But no matter the reason, if git describe
fails, we can't then call ./setup.py --version
, because the only time that can work is:
git describe
actually works, but then we wouldn't have this problem to start with (e.g./setup.py --version
doesn't get called)__version__
was already overwritten with a static value, in which case we didn't need to call./setup.py --version
in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I figured out how to load the version from setup.cfg with configparser instead, so I can avoid those circular calls. I'll try to fix that later and ask you to review it so I won't sneak in another bug for you 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge improvement 🎉
More than 10mb ram savings per extension.
Here is my functional, but not code complete, implementation of replacing web sockets over loopback network interface with unix sockets, to address #456 . I still want to do some more local stress testing before I say it is really ready, and I haven't even touched tests yet, but I wanted to post now to get some feedback on the approach. Also this is fully usable, with no changes needed to extensions, so anyone can try it out as well.
Since Ulauncher is already a Gtk based application, and thus has an included GLib event loop, I decided to leverage the capabilities included with GLib. This helps to push parts of the code down into the C library implementation of GLib, vs implementing the unix sockets with the python socket library. Unix sockets allow for better security since they can be created in user directories, with user permissions, thus leveraging the normal file system permissions for protection. Also, unix sockets have a bit less overhead even then local sockets, though this is unlikely to make a big impact in the Ulauncher case.
As part of making use of the event loop, I went on a bit of a thread slaying campaign, converting what I could from threads to events in the event loop. The hope was reduced potential race conditions and hopefully some reduced overhead. However, there are definitely a few
run_async
calls left that seem to be the easiest implementation, and unfortunately the Python threading library basically auto spawns a pool of threads on first use, so still a big pile of (presumably mostly idle) threads spawn off. Even so, I think many of the places I was able to convert into an event loop will generally help with the multi-thread nuances that can trip up code, so eliminating what I could still makes sense.Looking at memory consumption of the main Ulauncher process, and of the extension processes, I see about a 40% memory reduction. I'd be curious if that varies with different environments. The savings don't look quite as nice if the preferences dialog is opened because doing so more than doubles the memory usage, which isn't reclaimed when it is closed. This makes the memory savings a much smaller percentage.
I'm interested in feedback on this approach. Also, I recognize some of the increased usage of the GLib event loop is not strictly related to the unix socket implementation, so for the ultimate "real" PR I can make it more focused if you think it is more appropriate. Then I can propose some of the other conversions in separate PRs.
A quick overview of the changes:
Gtk.main()
in a separate thread, but with the ones I eliminated I was also able to drop that work around. I think it might have been the websocket server thread, but I'd have to do more experiments to confirm. Either way, the work around can be dropped.Sorry for the long message, but I thought it was good to provide the context and overview. As a final thought, I'm happy that this PR has a net removal of more than 400 lines, while increasing performance, reducing memory, and enhancing security. 😀