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

zero-copy support in cffi backend #1365

Merged
merged 9 commits into from
Jan 13, 2021
Merged

zero-copy support in cffi backend #1365

merged 9 commits into from
Jan 13, 2021

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Feb 25, 2020

When trying out PyPy2 with spyder via the spyder-kernels package (which is extends the ipython protocol?), I discovered that CFFI-based Frame was not working correctly: it did not support the buffer protocol. PyPy extends the ability of pure python classes to support the buffer protocol. If you inherit from the __pypy__.bufferable.bufferable class and then instaniate an instance of your class b, callng memoryview(b) will call self.__buffer__(flags) and give you a chance to return a buffer.

I implemented this, but it seems there is some code in the cython backend to release the buffer via a gc callback? In any case, I unskipped some of the tests.

Does this look OK? The only tests that are failing are the ones that check tracker.done, which makes me think maybe this is leaking memory.

@minrk
Copy link
Member

minrk commented Feb 26, 2020

This is great! Supporting the buffer interface isn't enough for the cffi backend to support zero-copy sends, though, which is what the done tests cover. This is still an improvement, so if you put back the skips on the two tracker tests, I think this is great.

In the pypy2 tests, it looks like numpy.frombuffer doesn't work with buffer-interface-providers:

B = numpy.frombuffer(msg, A.dtype).reshape(A.shape)
TypeError: expected string or Unicode object, Frame found

maybe need a version check or direct check for the presence of this bufferable type?

@mattip
Copy link
Contributor Author

mattip commented Feb 27, 2020

In the pypy2 tests, it looks like numpy.frombuffer doesn't work with buffer-interface-providers

There was a bug in PyPy, the fix should appear in the pypy2 nightly build tomorrow.

@mattip
Copy link
Contributor Author

mattip commented Feb 27, 2020

Supporting the buffer interface isn't enough for the cffi backend to support zero-copy sends

Is there something I can do to get this going?

@minrk
Copy link
Member

minrk commented Mar 3, 2020

Is there something I can do to get this going?

You could give it a go. To implement zero-copy, Frame needs to match the Cython version more, which you can use as reference.

Mainly:

  • send/Frame on a buffer with copy=False should not copy memory with zmq_msg_init_size, instead it should create a zmq_msg_t with zmq_msg_init_data referencing that memory and an associated GIL-less zmq_free_fn.
  • The zmq_free_fn sends a message to the garbage collector (which is in Python and thus shared between backends).

@mattip
Copy link
Contributor Author

mattip commented Mar 30, 2020

in a322aa0 I

  • created a separate build step for the cffi c-extension module
  • changed the cffi interfaces to use that module
  • copied the copy=False behaviour from message.pyx to message.py

Now test_multi_tracker in test_message.py hangs. How do I instrument the callbacks so I can see what is not being called? I don't see the fprintf output from the call to free_python_msg

setup.py Outdated

# whether any kind of bdist is happening
# do this before importing anything from distutils
doing_bdist = any(arg.startswith('bdist') for arg in sys.argv[1:])

if any(bdist in sys.argv for bdist in ['sdist', 'bdist_wheel', 'bdist_egg']):
import setuptools
from setuptools import setup
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any supported python should be able to use setuptools, and I think is to be preferred over distutils where possible

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're switching to requiring setuptools, let's make sure to avoid the main reason we haven't required setuptools: that it breaks python setup.py install by implicitly doing egg installs and corrupting sys.path:

from setuptools.command.bdist_egg import bdist_egg

class bdist_egg_disabled(bdist_egg):
    """Disabled version of bdist_egg

    Prevents setup.py install from performing setuptools' default easy_install,
    which it should never ever do.
    """

    def run(self):
        sys.exit(
            "Aborting implicit building of eggs. Use `pip install .` to install from source."
        )

...
setup_args['cmdclass'] = {
    ...
    'bdist_egg': bdist_egg if 'bdist_egg' in sys.argv else bdist_egg_disabled,
}

setup.py Show resolved Hide resolved
library_dirs=cfg['library_dirs'],
runtime_library_dirs=cfg['runtime_library_dirs'],
source="""
#include <stdio.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in a .c file instead of inlined?

setup.py Outdated

# whether any kind of bdist is happening
# do this before importing anything from distutils
doing_bdist = any(arg.startswith('bdist') for arg in sys.argv[1:])

if any(bdist in sys.argv for bdist in ['sdist', 'bdist_wheel', 'bdist_egg']):
import setuptools
from setuptools import setup
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're switching to requiring setuptools, let's make sure to avoid the main reason we haven't required setuptools: that it breaks python setup.py install by implicitly doing egg installs and corrupting sys.path:

from setuptools.command.bdist_egg import bdist_egg

class bdist_egg_disabled(bdist_egg):
    """Disabled version of bdist_egg

    Prevents setup.py install from performing setuptools' default easy_install,
    which it should never ever do.
    """

    def run(self):
        sys.exit(
            "Aborting implicit building of eggs. Use `pip install .` to install from source."
        )

...
setup_args['cmdclass'] = {
    ...
    'bdist_egg': bdist_egg if 'bdist_egg' in sys.argv else bdist_egg_disabled,
}

buildutils/build_cffi.py Outdated Show resolved Hide resolved
@minrk minrk force-pushed the pypy-cffi branch 2 times, most recently from 22fbd09 to 1f821b3 Compare January 13, 2021 12:58
- use manual malloc/free as ffi.new has the wrong lifecycle
- implement zero-copy recv
- get buffer from zmq_msg
@minrk
Copy link
Member

minrk commented Jan 13, 2021

zero-copy now works in PyPy. Thanks @mattip!

@minrk minrk changed the title WIP: PyPy can make a pure python class support the buffer protocol zero-copy support in cffi backend Jan 13, 2021
@minrk minrk merged commit 11ae0de into zeromq:master Jan 13, 2021
@mattip
Copy link
Contributor Author

mattip commented Jan 13, 2021

Thanks for finishing this up. Are there any benchmarks for pyzmq?

@mattip
Copy link
Contributor Author

mattip commented Jan 13, 2021

I found https://github.com/achimnol/asyncio-zmq-benchmark (that @minrk contributed to). Is that still relevant?

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.

None yet

2 participants