Api mode cffi compile #169

georgeharker · 2024-11-25T18:43:21Z

Cffi code is much faster when compiled with a c compiler (API mode) rather than using libfffi (ABI mode) - however this requires a compiler to be installed .

This change allows an install to be made using

XCFFIB_API_MODE=1 pip3 install xcffib

Various other projects (cairocffi, pangocffi and pangocairocffi) can then benefit from similar changes.

During installation XCFFIB_API_MODE=0 or XCFFIB_API_MODE not set defaults to the previous ABI mode install which incurs lever overhead on load, runs the ffi_build as before and will dynamically translate arguments to C using the general libffi.

During installationXCFFIB_API_MODE=1 compiles a shared library / C extension which dynamically depends on xcd.

At runtime if the shared library / C extension is present it will be used (unless XCFFIB_API_MODE=0). The user does not have to arrange for XCFFIB_API_MODE=1 to be set. If the extension is not present, the old behavior is used.

To make this work I modified the CPU checks to be portable, and added the relevant FFI calls / setup.py calls to have the C extension be built.

The language-python requirement changes may not be needed - I was working around a happy bug / clash which I think you've noted in a report for haskell.

I don't suggest this be default, but API mode is a potential substantial performance improvement (especially for cairo, which needs xcffib to be build in API mode to work, C and py ffi can't be mixed with ffi.include())

tych0

You'll be amused to know that just recently moved away from api mode (517169a) largely because it was too hard for people to install correctly.

I'm not opposed to supporting both modes, but we need to do it in a backwards compatible way, and probably have tests to make sure that when things are installed in either mode they still work.

xcffib.cabal

portable_nproc.sh

tych0 · 2024-11-25T20:46:29Z

module/ffi_build.py

+    )
+
+    if __name__ == "__main__":
+        ffi.compile(verbose=True)


This file rename looks like an API break, which will break downstream users, so we should figure out some way to avoid that.

due to the move? There was some oddity in that the ffi py name was shadowed by the ffi object in the module - which is why the rename - it seems like no one would have been able to access ffi.py without some module import path import lib magic. But I would definitely like it to be as compatible as possible. I believe in init.py I pull that back in as ffi in non api mode to make the change transparent externally

Yeah, that sounds good to me, thanks.

module/__init__.py

setup.py

tych0 · 2024-11-25T20:52:28Z

pyproject.toml

@@ -0,0 +1,3 @@
+[build-system]
+requires = ["setuptools >= 64"]
+build-backend = "setuptools.build_meta"


Can you elaborate on why this is necessary?

pyproject.toml is the defacto standard now and all projects are supposed to have one to select which build system is used - without it editable installs complain and will soon be disallowed (because the editable part needs to know which build system to use to install editable).

huh, I had never heard of "editable installs", TIL, thanks. Is there any chance we can completely get rid of setup.py? Last I had looked at pyproject.toml it wasn't fully feature parity with setup.py.

unfortunately in order to support stuff like api mode it's required, migration from .cfg to pyproject.toml is pretty doable, but feels like it might want to be separate to this PR.

setup.py

georgeharker · 2024-11-26T00:55:43Z

You'll be amused to know that just recently moved away from api mode (517169a) largely because it was too hard for people to install correctly.

I'm not opposed to supporting both modes, but we need to do it in a backwards compatible way, and probably have tests to make sure that when things are installed in either mode they still work.

I didn't;t know it used to be that way but can certainly understand that it's a bit of a pain - would love to provide the high performance option whilst preserving the easier route for folks by default.

Will respond to the rest inline.

tych0 · 2024-12-04T16:02:00Z

Ok, if you rebase this you shouldn't have the language-python based failures any more, I just wrote the code to drop our dependency on it. In no particular order, here are some things I'd like to see before landing this:

clean up the git history to standalone patches
do we need to add pyproject.toml to MANIFEST.in? I don't really know how that works.
I would like the (pytest) test suite to test both API and ABI mode based installs, to ensure we keep both working
have a module-level thing that indicates which mode you're running in, e.g. xcffib.cffi_mode == "API" or "ABI"

If we can do that, I think I'm happy to merge this.

One question is: should we have the env var at all? What about just figuring out if the user has a C compiler available and using it?

georgeharker · 2024-12-06T02:09:12Z

Great. I'll try and take a look at this in the next few days.

I'll try and find out if we need to add pyproject.toml to the manifest. It's possible not if the generated portion remains in setup.cfg. They can both be used and maybe modifying that would be best as a separate change?

It's definitely possible to just determine if the compiler is available - however I was concerned with the way installs work with pip - by default they will pull all dependencies fresh and build against the non installed version. I guess that would still work either way, perhaps better as it could ensure builds of the dependencies were made in api mode. I can experiment.

Will rebase against your changes!

The install mode global is a great idea. I'll propagate that onwards to cairocffi pangocffi and pangocairocffi.

georgeharker · 2024-12-06T23:43:32Z

Following up:

clean up the git history to standalone patches

I now have a pair commits and it's rebased off yours.

do we need to add pyproject.toml to MANIFEST.in? I don't really know how that works.

Checked on this and we should not. https://setuptools.pypa.io/en/latest/userguide/miscellaneous.html says it's included by default and would have to be explicitly excluded in MANIFEST.in. I have modified that to exclude the ABI mode product (see below)

I would like the (pytest) test suite to test both API and ABI mode based installs, to ensure we keep both working

Agreed - I'm a little confused about how to go about this - and also found difficulties in controlling setup.py in a fallback way (see below). It's possible we could reintroduce an env var to force a particular mode for testing?

have a module-level thing that indicates which mode you're running in, e.g. xcffib.cffi_mode == "API" or "ABI"
One question is: should we have the env var at all? What about just figuring out if the user has a C compiler available and using it?

At first I considered trying to have setup.py/setup() detect errors and fall back if compilation fails. This is fraught with issues and errors don't make it to exception handlers around setup() but rather break inside setup tools when invoked from pip.

After taking a step back I realized that ABI mode can also be precompiled which saves on import time.

This works like so:

setup() takes cffi_modules=["xcffib/ffi_build.py:ffi_build"] which says call ffi_build() from ffi_build.py. This function is expected to return an ffi on which compile() will be invoked by setuptools - however doing so is a no-op if it's already done. Instead, we try an attempt to make C compilation work, and fall back to generating _xcffib.py instead (which still requires dlopen() but skips all the cdefs as they're already done in a python-compiled way.

We also define some constants __XCFFIB_CFFI_MODE along with _XCFFIB_CFFI_MODE_API _XCFFIB_CFFI_MODE_ABI and _XCFFIB_CFFI_MODE_ABI_PRECOMPILED which allow the loaded ffi.lib to determine which mode was used. This is translated to xcffib.cffi_mode during module load ( the constants must be raw numbers, not strings etc).

What remains is to figure out a multi-mode test strategy.

georgeharker · 2024-12-07T02:02:11Z

I've noticed some issues with this approach (not so much for xcffib but for libraries wanting to build onto like cairocffi.

I will take a look at resolving these in a way that allows those to be build api mode - it basically hangs on being able to grab the non compiled ffi from ffi_build but i don't want that to parse cdefs multiple times. Just a little fiddly, but i'm sure doable.

georgeharker · 2024-12-07T06:57:47Z

I had originally tried defining constants in the complex xcffi library. Which is doable but in so doing I had redefined the ffi for each mode which was inefficient. I also realized I could do away with that entirely and just determine which is which in init mode.

After a bit of restructuring I think I have a workable solution which allows packages which depend on xcffib to pull the api version of the ffi to use as an include on their ffi.

It seems to work well. I haven't added tests yet.

Current state is in the PR. I'll do further checks on cairocffi to make sure this works as needed downstream.

If you have suggestions on how to write tests for all modes I'd be interested.

This should be pretty transparent for users. It should even work without running setup from the unpacked source.

tych0

Thanks, this is looking pretty nice, a few small comments.

In terms of testing, however we can make sure CI runs both targets is fine with me. Here is one (though perhaps not the best/only) way I can imagine it working:

change the xcffib build target to also run xcffib/ffi_build.py
change CI to install a C compiler, then have it do make check && apt-get autoremove <the C compiler> && make clean && make check

That way we can test both modes. Ideally make check would check both and we wouldn't have to uninstall the C compiler, but I'm not really sure how to mask it when it's still installed. maybe CC=/bin/false is enough? not sure.

tych0 · 2024-12-09T15:44:07Z

module/ffi_build.py

+        ffi_api.compile(verbose=True)
+        return ffi_api
+    except Exception:
+        warn("Falling back to precompiled python mode")


Does this need to be a warn()?

the warning seems useful from a debug perspective - it's actually hard to get things to show up from the build subprocesses and by default nothing will show for pip install unless pip install -v is used. Can still change if desired.

module/ffi_build.py

module/__init__.py

georgeharker · 2024-12-09T16:48:44Z

Thanks, this is looking pretty nice, a few small comments.

In terms of testing, however we can make sure CI runs both targets is fine with me. Here is one (though perhaps not the best/only) way I can imagine it working:

change the xcffib build target to also run xcffib/ffi_build.py

change CI to install a C compiler, then have it do make check && apt-get autoremove <the C compiler> && make clean && make check

That way we can test both modes. Ideally make check would check both and we wouldn't have to uninstall the C compiler, but I'm not really sure how to mask it when it's still installed. maybe CC=/bin/false is enough? not sure.

It turns out distutils uses the env var CC, so as you suggest, CC=/bin/false would do the trick. I'll take a stab at modifying the .github cc - would you be able to look at that when done?

I'm not actually sure if we are running pip install on the package before testing - so I suspect we are already falling back to out of line abi mode. In which case the simplest may be to do a series of pytest with a build in-between of the ffi using CC=/bin/false python3 xcffib/ffi_build.py and then python3 xcffib/ffi_build.py

georgeharker · 2024-12-09T17:02:30Z

I've modified the makefile to check all modes.

tych0 · 2024-12-09T20:46:55Z

 # check abi precompiled mode
CC=/bin/false python3 xcffib/ffi_build.py
Traceback (most recent call last):
  File "/home/runner/work/xcffib/xcffib/xcffib/ffi_build.py", line 16, in <module>
    from distutils.errors import CCompilerError, DistutilsExecError, DistutilsPlatformError
ModuleNotFoundError: No module named 'distutils'

on newer pythons, which makes sense, since:

    from distutils.errors import CCompilerError, DistutilsExecError, DistutilsPlatformError

seems to happen on older pythons. I see:

Traceback (most recent call last):
  File "/home/tycho/packages/qtile/qtile_venv/lib/python3.13/site-packages/setuptools/_distutils/spawn.py", line 70, in spawn
    subprocess.check_call(cmd, env=_inject_macos_ver(env))
    ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.13/subprocess.py", line 419, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/usr/bin/false', '-fno-strict-overflow', '-Wsign-compare', '-DNDEBUG', '-g', '-O2', '-Wall', '-g', '-fstack-protector-strong', '-Wformat', '-Werror=format-security', '-g', '-fwrapv', '-O2', '-fPIC', '-I/home/tycho/packages/qtile/qtile_venv/include', '-I/usr/include/python3.13', '-c', '_xcffib.c', '-o', './_xcffib.o']' returned non-zero exit status 1.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/tycho/packages/qtile/qtile_venv/lib/python3.13/site-packages/setuptools/_distutils/unixccompiler.py", line 200, in _compile
    self.spawn(compiler_so + cc_args + [src, '-o', obj] + extra_postargs)
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tycho/packages/qtile/qtile_venv/lib/python3.13/site-packages/setuptools/_distutils/ccompiler.py", line 1045, in spawn
    spawn(cmd, dry_run=self.dry_run, **kwargs)
    ~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tycho/packages/qtile/qtile_venv/lib/python3.13/site-packages/setuptools/_distutils/spawn.py", line 76, in spawn
    raise DistutilsExecError(
        f"command {_debug(cmd)!r} failed with exit code {err.returncode}"
    ) from err
distutils.errors.DistutilsExecError: command '/usr/bin/false' failed with exit code 1

So I wonder if we can directly import it somehow? I played around a bit and couldn't figure it out.

georgeharker · 2024-12-09T20:53:35Z

Evidently the thing to do is to import setup tools which we already rely on and sub imports / shims those errors. Updated the PR.

georgeharker · 2024-12-09T21:43:12Z

Evidently the thing to do is to import setup tools which we already rely on and sub imports / shims those errors. Updated the PR.

Ive fixed the derived imports which change name slightly. This appear to now work.

georgeharker · 2024-12-10T00:19:28Z

Evidently the thing to do is to import setup tools which we already rely on and sub imports / shims those errors. Updated the PR.

Ive fixed the derived imports which change name slightly. This appear to now work.

CI checks are failing for reasons I don't understand - looks like missing xcb and cffi?

tych0 · 2024-12-10T00:25:21Z

Makefile

+	python3 -c "import xcffib; assert xcffib.cffi_mode == 'abi_precompiled'"
+	pytest-3 -v --durations=3 -n auto
+	# check api mode
+	python3 xcffib/ffi_build.py


This seems like it generates stuff in the root directory, shouldn't it really live in module/? If not, why not? Can we add it to the clean target if not?

It does. As does the setup which would install those libs). But it is doable to have them built in xcffib/ and to adjust adjust the import paths in __init__.py. I’ll do that and write back.

This now compiles in temp and copies in. The extensions will be under xcffib/

tych0 · 2024-12-10T00:32:09Z

CI checks are failing for reasons I don't understand - looks like missing xcb and cffi?

Yeah, very odd. The pytest-3 call right above that works, which should run the very same import line, IIUC.

georgeharker · 2024-12-10T18:35:51Z

CI checks are failing for reasons I don't understand - looks like missing xcb and cffi?

Yeah, very odd. The pytest-3 call right above that works, which should run the very same import line, IIUC.

I've modified the PR to include a more isolated environment and try to set the PYTHON var used for python in the workflow.

tych0 · 2024-12-10T19:29:51Z

Yeah, I think this test failure is my fault, let me see if I can figure it out.

tych0 · 2024-12-10T19:55:40Z

Bit of an embarrassing bug :). Can you see if #172 fixes it for you?

georgeharker · 2024-12-11T21:50:06Z

Overall looks pretty good to me. Two minor comments, and then if you clean up the git history, I'm happy to merge + release.

Fantastic! Resolved the comments and have squashed the commits for a clean history.

tych0 · 2024-12-11T21:56:58Z

I think it'll fail on pushd & popd too, or at least it did for me locally:

pushd /tmp/tmp.PCgMxGyCa6 && \
	/tmp/tmp.PCgMxGyCa6/bin/python3 -c "import xcffib; assert xcffib.cffi_mode == 'api'" && \
	popd
/bin/sh: 1: pushd: not found
make: *** [Makefile:96: check-api] Error 127

I think it's fine to make this into a script that we call, vs. having line-continuation-make nonsense.

georgeharker · 2024-12-11T22:08:29Z

I think it'll fail on pushd & popd too, or at least it did for me locally:
pushd /tmp/tmp.PCgMxGyCa6 && \
	/tmp/tmp.PCgMxGyCa6/bin/python3 -c "import xcffib; assert xcffib.cffi_mode == 'api'" && \
	popd
/bin/sh: 1: pushd: not found
make: *** [Makefile:96: check-api] Error 127
I think it's fine to make this into a script that we call, vs. having line-continuation-make nonsense.

I have an easy fix and will upload

georgeharker · 2024-12-11T22:34:23Z

latest fails may be prior to my change to have the outputs copied individually? I don't see the copy lines in the logs. Wondering if I'm missing something.

tych0 · 2024-12-11T22:38:51Z

I think it's because both check-api and check-abi really have a dependency on xcffib, but it's not declared in the current makefile? or at least, I hope that's it :)

georgeharker · 2024-12-11T22:41:29Z

good catch - thanks, have fixed

georgeharker · 2024-12-12T05:13:23Z

There's still a race in the Makefile which I'm tracking down, it's the root of the failures. Have recreated here.

georgeharker · 2024-12-12T15:43:58Z

Gah! How frustrating. This works locally. I'm fairly convinced this is a race. But I'm not quite sure why. I'm going to run this under Debian and try and figure it out.

tych0 · 2024-12-12T15:54:30Z

Classic computers. I'm afk for the next few days, but I can take a look early next week too.

attempt to compile xcffib by default. This makes a pre-parsed ffi we can import without parsing cdefs at import time. If c compilation API mode fails, fall back to ABI mode but still precompile. Finally if the compiled library is missing for some reason fall back to import time ABI mode. xcffib.cffi_mode reflects which option was used.

georgeharker · 2025-01-08T16:37:34Z

Thanks for your efforts on this!

tych0 · 2025-01-08T16:42:59Z

Yeah, sorry it's taken me a while to get to it. I hope I'm pretty close... let me know what you think of that patch and I can fold it in and clean it up as appropriate once I get it working.

georgeharker · 2025-01-08T20:50:20Z

It looks great to me - and I think using sh will be a lot cleaner and avoid the race I had!

...this way we can actually compile for api mode again. Signed-off-by: Tycho Andersen <[email protected]>

tych0 · 2025-01-08T21:55:49Z

Ok, looks like we still have one race, but I don't really want to fight it that hard. If I can't figure it out, I'll just rip out parallel test execution, it's not that bad to have them go serially.

One question I have, that maybe we talked about before but I have since forgotten is: under what circumstances does abi mode get used? abi_precompiled is when the user didn't have a C compiler during installation (which... isn't _precompiled a misnomer there? shouldn't that really just be abi?), but when does abi get used?

tych0 · 2025-01-08T21:56:00Z

In particular, I would like to test it, but I don't understand how.

georgeharker · 2025-01-08T23:16:05Z

I'm so confused as to why it clashes like that. Thanks for looking into it.

abi_precompiled corresponds to this doc https://cffi.readthedocs.io/en/stable/overview.html#out-of-line-abi-level a It works by making a python lib that can be imported and uses ffilib which is slower than c. It's precompiled in the sense that the Cdef parsing has been done and made into opaque structures in the python lib which gets imported.

abi mode is this one https://cffi.readthedocs.io/en/stable/overview.html#simple-example-abi-level-in-line should make it possible to download the library fresh and import it without install - in which case there would be no share lib made (no setup tools install has run). I think a fresh checkout (with python gen run) would do the trick and an import on that library. It will parse cdefs at import time.

tych0 · 2025-01-09T16:06:15Z

should make it possible to download the library fresh and import it without install

I am curious: how would one do that? When we ship the thing via pypi, the setuptools cffi bits will (presumably) always run, and so this mode is really only for development, IIUC? If so, since we're not really using it for local testing any more, I wonder if we should fail here and declare it as explicitly unsupported since we can't test it with the new install-to-test scheme.

This was referenced Nov 25, 2024

Api mode cffi compile Kozea/cairocffi#230

Open

Api mode cffi compile leifgehrmann/pangocairocffi#34

Open

Api mode cffi compile leifgehrmann/pangocffi#61

Open

tych0 reviewed Nov 25, 2024

View reviewed changes

georgeharker force-pushed the api-mode branch 2 times, most recently from 386aa5a to 2d650b0 Compare December 6, 2024 23:30

georgeharker force-pushed the api-mode branch 3 times, most recently from 6af8cda to 012b318 Compare December 7, 2024 00:57

georgeharker force-pushed the api-mode branch 2 times, most recently from f177735 to 08e767e Compare December 7, 2024 05:27

tych0 reviewed Dec 9, 2024

View reviewed changes

georgeharker force-pushed the api-mode branch from 18f7c05 to 3976e04 Compare December 9, 2024 21:26

tych0 reviewed Dec 10, 2024

View reviewed changes

georgeharker force-pushed the api-mode branch 2 times, most recently from 600f3f9 to 5689a2d Compare December 11, 2024 21:54

georgeharker added 4 commits January 7, 2025 11:12

make abi and api checks one line

336ac4a

add dependency on xcffib for check-api and check-abi

bfd2c39

attempt to fix makefile race and remove command unneeded chaining

2213aff

tych0 force-pushed the api-mode branch from 0568690 to 2213aff Compare January 7, 2025 18:12

wip

e5211d4

tych0 force-pushed the api-mode branch from 5bde6d2 to e5211d4 Compare January 8, 2025 16:33

tych0 force-pushed the api-mode branch 2 times, most recently from b529d72 to c2ce3b7 Compare January 8, 2025 21:11

tych0 added 2 commits January 8, 2025 14:20

wip

350acb8

ci: install build-time deps

1c3f176

...this way we can actually compile for api mode again. Signed-off-by: Tycho Andersen <[email protected]>

tych0 force-pushed the api-mode branch from c2ce3b7 to 1c3f176 Compare January 8, 2025 21:21

Api mode cffi compile #169

Are you sure you want to change the base?

Api mode cffi compile #169

Conversation

georgeharker commented Nov 25, 2024

tych0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeharker commented Nov 26, 2024

tych0 commented Dec 4, 2024

georgeharker commented Dec 6, 2024

georgeharker commented Dec 6, 2024 • edited Loading

georgeharker commented Dec 7, 2024

georgeharker commented Dec 7, 2024

tych0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeharker commented Dec 9, 2024 • edited Loading

georgeharker commented Dec 9, 2024

tych0 commented Dec 9, 2024

georgeharker commented Dec 9, 2024 • edited Loading

georgeharker commented Dec 9, 2024

georgeharker commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tych0 commented Dec 10, 2024

georgeharker commented Dec 10, 2024

tych0 commented Dec 10, 2024

tych0 commented Dec 10, 2024

georgeharker commented Dec 11, 2024

tych0 commented Dec 11, 2024

georgeharker commented Dec 11, 2024

georgeharker commented Dec 11, 2024

tych0 commented Dec 11, 2024

georgeharker commented Dec 11, 2024

georgeharker commented Dec 12, 2024

georgeharker commented Dec 12, 2024

tych0 commented Dec 12, 2024

georgeharker commented Jan 8, 2025

tych0 commented Jan 8, 2025

georgeharker commented Jan 8, 2025

tych0 commented Jan 8, 2025

tych0 commented Jan 8, 2025

georgeharker commented Jan 8, 2025

tych0 commented Jan 9, 2025

georgeharker commented Dec 6, 2024 •

edited

Loading

georgeharker commented Dec 9, 2024 •

edited

Loading

georgeharker commented Dec 9, 2024 •

edited

Loading