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

cythonize more #3978

Closed
totaam opened this issue Aug 28, 2023 · 8 comments
Closed

cythonize more #3978

totaam opened this issue Aug 28, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@totaam
Copy link
Collaborator

totaam commented Aug 28, 2023

The goal is similar to #1953 and #1700.
Now with the added benefit that type hints can (almost) be used by Cython to compile plain python code.
Also review the reports for existing performance critical cythonized code to ensure that there is no Python interaction.

@totaam
Copy link
Collaborator Author

totaam commented Aug 28, 2023

One common problem is the use of the f-string {varname=} format with older versions of Cython:

[1/1] Cythonizing /home/antoine/projects/xpra/xpra/x11/xsettings_prop.py

Error compiling Cython file:
------------------------------------------------------------
...
            add((red, blue, green, alpha))
            pos += 8
        else:
            log.error("invalid setting type: %s, cannot continue parsing XSETTINGS!", setting_type)
            break
    log(f"bytes_to_xsettings(..) {serial=} ,{settings=}")
                                          ^

Another is from __future__ import annotations:

[1/1] Cythonizing /home/antoine/projects/xpra/xpra/net/file_transfer.py

Error compiling Cython file:
------------------------------------------------------------
...
# Copyright (C) 2010-2023 Antoine Martin <[email protected]>
# Copyright (C) 2008, 2010 Nathaniel Smith <[email protected]>
# Xpra is released under the terms of the GNU GPL v2, or, at your option, any
# later version. See the file COPYING for details.

from __future__ import annotations
                      ^
------------------------------------------------------------

xpra/net/file_transfer.py:7:23: future feature annotations is not defined

totaam added a commit that referenced this issue Aug 30, 2023
totaam added a commit that referenced this issue Sep 20, 2023
totaam added a commit that referenced this issue Sep 20, 2023
@totaam
Copy link
Collaborator Author

totaam commented Sep 20, 2023

It takes a while to run, but cythonize is now happy to run on all the files:

find ./xpra -name "*.py" -exec python3.10-cythonize -3 {} \;

So rather than cythonizing everything, I've selected the classes that are the most likely to benefit from cythonization:

  • network handlers
  • pure-python image functions
  • client window backing

This does flag some bugs which is useful in itself.
Perhaps this should have different levels, ie: --with-cythonize_more=9

@totaam
Copy link
Collaborator Author

totaam commented Sep 24, 2023

Found a few more bugs in the process. (ie: xpra top :DISPLAY still hangs when cythonized..)
MS Windows can almost be tested using BUILD_OPTIONS="--with-cythonize_more" ./packaging/MSWindows/MINGW_BUILD.sh but MSYS2 is still shipping Cython 0.x and compilation takes a looong time on this build VM.

totaam added a commit that referenced this issue Sep 25, 2023
totaam added a commit that referenced this issue Sep 25, 2023
totaam added a commit that referenced this issue Sep 25, 2023
totaam added a commit that referenced this issue Sep 25, 2023
totaam added a commit that referenced this issue Sep 25, 2023
@totaam
Copy link
Collaborator Author

totaam commented Sep 26, 2023

Server startup time without extra cythonization is already plenty fast, much faster than when #2341 / #2815 were an issue.

The only modules that don't take less than 1ms to initialize are:

2023-09-26 15:42:25,501    2ms in <class 'xpra.server.mixins.input.InputServer'>.init
2023-09-26 15:42:25,596   95ms in <class 'xpra.server.mixins.encoding.EncodingServer'>.init
2023-09-26 15:42:25,605    8ms in <class 'xpra.server.mixins.window.WindowServer'>.init
2023-09-26 15:42:25,697   21ms in <class 'xpra.server.server_core.ServerCore'>.setup
2023-09-26 15:42:25,701    3ms in <class 'xpra.server.mixins.notification.NotificationForwarder'>.setup
2023-09-26 15:42:25,707    5ms in <class 'xpra.server.mixins.clipboard.ClipboardServer'>.setup
2023-09-26 15:42:33,108 7399ms in <class 'xpra.server.mixins.encoding.EncodingServer'>.setup

(+- 10% variation)
So we're waiting for IO (talking to the X11 server) for input, clipboard, notification and encoding.
ServerCore is also doing IO for setting up the sockets.
Unsurprisingly, cythonizing these modules makes no difference.


Not a single one of the client modules (#2347) takes more than 1ms!

On my system the only two things slowing down client startup are:

  • audio probing: ~400ms
  • CUDA initialization: ~600ms

One issue unrelated to cythonization remains:

2023-09-26 15:42:32,117 loaded 96 start menu entries from 13 sub-menus in 6.4 seconds

(and this should not delay client connections since we now require the xdg-menu update feature)


This extra cythonization is working so well that it could be made the default in the future.

totaam added a commit that referenced this issue Sep 26, 2023
@totaam
Copy link
Collaborator Author

totaam commented Sep 27, 2023

Massive related tidy up: cbbea30

totaam added a commit that referenced this issue Sep 27, 2023
move win32 service to a more logical place so the automagic can do its thing
totaam added a commit that referenced this issue Sep 28, 2023
totaam added a commit that referenced this issue Sep 28, 2023
totaam added a commit that referenced this issue Sep 28, 2023
totaam added a commit that referenced this issue Sep 29, 2023
totaam added a commit that referenced this issue Sep 29, 2023
and update some dialog paths
@totaam
Copy link
Collaborator Author

totaam commented Sep 29, 2023

A standard Fedora build now cythonizes over 400 files!
This takes too long..
On the plus side, almost all unit tests pass.
Packaging could be difficult as rpmbuild spills out lots of:

RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/codecs/checks.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/codecs/constants.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/codecs/debug.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/codecs/icon_util.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/codecs/image.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/codecs/loader.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/codecs/rgb_transform.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/codecs/video.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/common.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/exit_codes.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/log.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib/debug/usr/lib64/python3.11/site-packages/xpra/os_util.cpython-311-x86_64-linux-gnu.so-6.0-10.r34426.fc37.x86_64.debug
   /usr/lib64/python3.11/site-packages/xpra/codecs/checks.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/codecs/constants.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/codecs/debug.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/codecs/icon_util.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/codecs/image.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/codecs/loader.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/codecs/rgb_transform.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/codecs/video.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/common.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/exit_codes.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/log.cpython-311-x86_64-linux-gnu.so
   /usr/lib64/python3.11/site-packages/xpra/os_util.cpython-311-x86_64-linux-gnu.so

This will do for now.

@totaam
Copy link
Collaborator Author

totaam commented Jan 1, 2024

This caused a very unusual bug!

Traceback (most recent call last):
  File "xpra/net/socket_util.py", line 129, in xpra.net.socket_util.add_listen_socket
    from xpra.net.quic.listener import listen_quic
  File "xpra/net/quic/listener.py", line 26, in init xpra.net.quic.listener
    from xpra.net.quic.websocket import ServerWebSocketConnection
  File "xpra/net/quic/websocket.py", line 19, in init xpra.net.quic.websocket
    log = Logger("quic")
  File "xpra/log.py", line 401, in xpra.log.Logger.__init__
    self.debug("debug enabled for %s / %s", caller, categories)
  File "xpra/log.py", line 446, in xpra.log.Logger.debug
    self.log(logging.DEBUG, msg, *args, **kwargs)
  File "xpra/log.py", line 439, in xpra.log.Logger.log
    global_logging_handler(self._logger.log, self.level_override or level, msg, *args, **kwargs)
  File "xpra/log.py", line 106, in xpra.log.standard_logging
    log(level, msg, *args, **kwargs)
  File "/usr/lib64/python3.12/logging/__init__.py", line 1609, in log
    self._log(level, msg, args, **kwargs)
  File "/usr/lib64/python3.12/logging/__init__.py", line 1684, in _log
    self.handle(record)
  File "/usr/lib64/python3.12/logging/__init__.py", line 1700, in handle
    self.callHandlers(record)
  File "/usr/lib64/python3.12/logging/__init__.py", line 1762, in callHandlers
    hdlr.handle(record)
  File "xpra/log.py", line 462, in xpra.log.Logger.handle
    self.log(record.levelno, record.msg, *record.args, exc_info=record.exc_info)
  File "xpra/log.py", line 439, in xpra.log.Logger.log
    global_logging_handler(self._logger.log, self.level_override or level, msg, *args, **kwargs)

(...)

RecursionError: maximum recursion depth exceeded

It was difficult to debug, the problem comes from this line:

if caller and caller != "__main__" and not caller.startswith("importlib"):

The cythonized version ends up not inserting a category for the caller (which is importlib._bootstrap), and so the aioquic logger is the same as the internal logger and it ends up calling itself.

totaam added a commit that referenced this issue Jan 1, 2024
@totaam
Copy link
Collaborator Author

totaam commented Jan 20, 2024

Another downside of cythonize_more is that some exceptions and deprecation warnings end up being reported from the last pure-python caller instead of the cythonized module.

An deprecation warning that seemed to be coming from threading.Thread actually came from:

/usr/lib64/python3.13/site-packages/xpra/util/version.py:257: DeprecationWarning: 'count' is passed as positional argument
  return re.sub(".*model name.*:", "", line,1).strip()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant