Synapse on PyPy

[Valodim]

This is a stack of patches I applied to Synapse to be able to run it on the PyPy runtime. The amount of changes is actually quite small, I commented in the individual commits. I ran my production instance (something like 20 active users I'd estimate, and in a couple big rooms too) for two days on PyPy 5.6.0 with this, without a hitch 👍

All unit tests pass, except for image preview ones which I didn't test. The reason is that these require lxml, which can be installed for pypy but not trivially. Tracking continuous compatibility with tox should also be fairly simple.

I would say that both memory and performance were about even. Memory usage might have been slightly higher than on CPython, but it's hard to tell since memory usage depends so much on forward extremities and regularly explodes for me on CPython as well.

Feel free to merge, cherry-pick, or close. If there is interest I'd be happy to add a paragraph to the readme, which I guess mostly boils down to "pypy also works! please test, measure, and report".

[matrixbot]

Can one of the admins verify this patch?

[matrixbot]

Can one of the admins verify this patch?

[superboum]

I did some research, didn't find your issue and came with a similar (less elegant) patch ( master...superboum:superboum/pypy ) and finally found your issue...

The next step I think, which is not so simple, would be to evaluate precisely what is the gain (if any).
I think measuring each request duration (from the frontend - like nginx) for example could be a good starting point. I will see what I can do but don't expect anything ;)

On some pure computation problem, I had a 5x gain by switching from cpython to pypy. But here, it's a bit more complex and not purely computanional.

Anyway, if the travis build does not pass, that's is for the following reason:

synapse/storage/engines/__init__.py:36:17: E128 continuation line under-indented for visual indent

[superboum]

This line fails the build because it breaks the linter

[ara4n]

If anyone has quantitive measures on how much faster or less/more RAM this makes synapse go, we would be all ears (and consider merging it)

[Valodim]

I won't have time to look into this further. However, I'd point out that merging this should have no adverse effects on cpython at all, while making the lives easier for people who want to try and experiment with pypy. Officially supporting pypy is a different beast, of course.

[richvdh]

could you explain why the cast to str is required under psycopg2cffi? this is such a hot path that I'm reluctant to add anything that isn't absolutely vital.

[Valodim]

I'm not 100% sure why this is necessary: The value returned here by psycopg2cffi comes from ffi.string, which for some reason couldn't be interned as-is. I couldn't find anything on google regarding ffi.string vs intern, and casting to str was a simple fix. Could be it returns a unicode object, but PQfname is documented to return char* and not wchar_t* so if it does I don't know why :)

If this is performance-critical, might be worth checking the actual runtime type. Although actually cursor_to_dict might be conceptually suboptimal if this is really on a super hot path?

[richvdh]

Thanks for doing this work, @Valodim, and sorry for sitting on it for months. As you rightly say there doesn't seem to be much disadvantage to landing these patches to help people experiment with pypy.

Would you be able to sign off the PR as per https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst ?

[richvdh]

@matrixbot: test this please

@Valodim: any chance you can sign off the PR (or each of the commits)?

[Valodim]

Signed-off-by: Vincent Breitmoser [email protected]

[richvdh]

That, sir, is an awesome email address.

[richvdh]

[part of this has been reverted by d3347ad ]

[Valodim]

Ugh, this is one of these situations where sortedcontainers probably meant to work as a drop-in, but now they couldn't change even if they wanted to because some users expect the method to work that way. Dang.

Nice catch, hope it didn't eat too much of your time

[richvdh]

I've opened #3098 to track redoing this

[luminoso]

Is this still working? I tested within this environment:

Twisted==18.9.0
matrix-synapse==0.33.8
PyPy 6.0.0/Python 2.7.13

And I had an exception under Twisted

ERROR - - File "/var/lib/synapse/pypy_venv/site-packages/twisted/enterprise/adbapi.py", line 211, in init
ERROR - - self.dbapi = reflect.namedModule(dbapiName)
ERROR - - File "/var/lib/synapse/pypy_venv/site-packages/twisted/python/reflect.py", line 162, in namedModule
ERROR - - topLevel = import(name)
ERROR - - ImportError: No module named psycopg2

I made a very ugly patch to "/var/lib/synapse/pypy_venv/site-packages/twisted/python/reflect.py"_, line 162 by adding as documented in psycopg2cffi project:

from psycopg2cffi import compat
compat.register()

like so:

def namedModule(name):
    """
    Return a module given its name.
    """
    from psycopg2cffi import compat
    compat.register()

    topLevel = __import__(name)

    packages = name.split(".")[1:]
    m = topLevel
    for p in packages:
        m = getattr(m, p)
    return m

And everything works smoothly. Did i miss anything or was some patch reverted?

[Valodim]

Seems like the psycopg2cffi patch I made doesn't affect the adbapi setup, since it was added at a later point. As an alternative patch to the one you have, can you try modifying synapse's server.py line 363 like this?:

    def build_db_pool(self):
        name = self.db_config["name"]

        import platform
        if (name == "psycopg2" and
                platform.python_implementation() == "PyPy"):
            name = "psycopg2cffi"

I think #3098 is no longer relevant, and hence synapse should run just fine on PyPy. Please do report your experience if you keep running synapse on pypy for a while. If we can get pypy into the tox matrix, synapse's upstream support should become much more stable 👍

(On a related note, synapse might benefit from moving to txpostgres. It hasn't been around quite as long as adbapi, but is considered stable and might offer better performance since it actually uses the libpq async api to implement deferreds, rather than a blocking connection pool of python threads)

[luminoso]

That patch did solve the problem.

I've been running it under Pypy with no other issues so far. It is running just fine.

(Regarding txpostgres I didn't give it a try, yet. I want to make sure that running under Pypy is stable without adding more variables)

[Valodim]

continued in #4174