move ffi module to separate crate #2126

pascalkuthe · 2022-01-25T14:56:56Z

This PR moves the ffi bindings to a seperate crate so they can be used independently from the rest of the crate.
The motivation here is that some special use cases might want to use the ffi bindings directly without touching the rest of pyo3.
Elimination of the dependency on the rest of pyo3 makes for much easier auditing and can improve compile times:
For my project this this reduces compile times for debug build from ~6 seconds to ~3 seconds. Note that the macros feature was not used in this comparison. I am using a fairly beefy CPU (8 core ryzen). On a laptop the improvements might be even larger.

Open Questions

There are some open questions on my side. I am not sure how to handle these as I am not as familiar with the pyo3 codebase

There is quite a lot of duplication between the buildscripts of pyo3-ffi and pyo3. Is there a way to address this?
Some tests and the datetime componenent of the ffi bindings had to remain in pyo3 itself due to integration with GILCell. Is this the desired solution?

mejrs

Thanks! From glancing through this I have a few questions and comments.

Some tests and the datetime componenent of the ffi bindings had to remain in pyo3 itself due to integration with GILCell. Is this the desired solution?

Perhaps we can have some sort of "raw api" in pyo3-ffi, with pyo3 offering a safe wrapper?

I also noticed its readme: https://github.com/DSPOM2/pyo3/blob/main/pyo3-ffi/README.md

I know you just moved it, but this is pretty outdated and the link is broken. Would you be able to fix it? :)

pyo3-ffi/src/pyerrors.rs

src/ffi/mod.rs

pyo3-ffi/src/lib.rs

davidhewitt · 2022-01-25T18:06:31Z

For my project this this reduces compile times for debug build from ~6 seconds to ~3 seconds.

Is that for a clean or incremental build?

pascalkuthe · 2022-01-25T18:09:05Z

For my project this this reduces compile times for debug build from ~6 seconds to ~3 seconds.

Is that for a clean or incremental build?

This is for a clean build. I wouldn't expect huge changes for an incremental build (after all pyo3 is not rebuild in that case)

pascalkuthe · 2022-01-25T18:10:18Z

I know you just moved it, but this is pretty outdated and the link is broken. Would you be able to fix it? :)

I will adress that along with your other comments :)

davidhewitt · 2022-01-25T18:17:18Z

This is for a clean build. I wouldn't expect huge changes for an incremental build (after all pyo3 is not rebuild in that case)

Understood! And is that 3 seconds difference just between pyo3 and pyo3-ffi, or is that the whole compile including dependencies like parking_lot which aren't needed any more?

pascalkuthe · 2022-01-25T18:38:24Z

And is that 3 seconds difference just between pyo3 and pyo3-ffi, or is that the whole compile including dependencies like parking_lot which aren't needed any more?

The times I reported above were just how long I had to wait for cargo build to finish so it included the dependencies (after a cargo clean command).
But because I have very few dependencies and 16 threads cargo is able to compile all of them in parallel so the 3 seconds are mostly due to pyo3 itself. To get a clearer picture I decided to do some tests with cargo bloat.

Both tests were performed after a clean build with the following command:
Note that I patched my crates.io setup to use my github fork instead of the version pubished on crates.io (that's the reason pyo3_ffi shows up in both examples)

cargo bloat -p verilogae_py --time -j 1

Compiled with pyo3

 Time Crate

4.98s pyo3
2.63s pyo3_build_config
0.98s libc
0.95s verilogae_py
0.78s pyo3_ffi
0.58s parking_lot_core
0.49s parking_lot
0.33s verilogae_ffi
0.20s once_cell
0.19s smallvec
0.19s lock_api
0.05s instant
0.04s scopeguard
0.02s cfg_if

Compiled with pyo3-ffi

 Time Crate

2.61s pyo3_build_config
1.04s verilogae_py
1.01s libc
0.78s pyo3_ffi
0.33s verilogae_ffi
0.20s once_cell

davidhewitt · 2022-01-25T18:50:12Z

Thanks! I'm reasonably convinced that if we can make the split tidy this will be helpful for projects like yours 👍

pascalkuthe · 2022-01-25T21:04:33Z

Perhaps we can have some sort of "raw api" in pyo3-ffi, with pyo3 offering a safe wrapper?

I created such a raw API. Instead of a GILCell this API uses a mutable static (like the original c macros in cpython). For backwards compatibility I then shadow this static with a wrapper that ensure that the datetime api is imported before use. Furthermore I reexport all functions/c macros that depends on this static in the ffi module. These shadowed implementations ensure that the API is imported (like the previous implementations did automatically with deref) and then call the raw C API. This should retain backwards compatibility while still exposing all raw functionality in pyo3-ffi.

The implementation should compile down to basically the same thing as previously but its still very unsafe and it would be good if someone more familiar with the codebase checked this thoroughly.

pascalkuthe · 2022-01-25T21:09:41Z

While writing the example for the documentation I noticed that the definition of _PyCFunctionFast was incorrect. The argument kwnames: *mut PyObject is only present in the PyCFunctionWithKeywords function according to the docs (and that makes sense if you look at the name) but in pyo3 it was also present in the _PyCFunctionFast definition. I included a small fix for this as a separate commit. I use _PyCFunctionFast in the docs so so the commit is somewhat related. If this is undesired I can drop the commit and create a separate PR

pascalkuthe · 2022-01-25T21:29:40Z

I have moved most of the buildscript to pyo3-ffi as that is where all the linking takes places (this is what was causing the build failures on windows I believe). This is now ready for review

adamreichold · 2022-01-25T22:06:39Z

Instead of a GILCell this API uses a mutable static (like the original c macros in cpython).

In light of rust-lang/rust#53639, UnsafeCell might be preferable to static mut?

pascalkuthe · 2022-01-25T22:46:59Z

In light of rust-lang/rust#53639, UnsafeCell might be preferable to static mut?

While I am pretty sure this implementation is safe (as there are essentially only ever reads and writes and no references are ever created) providing a cell would potentially be better API for downstream crates. I will take a look at this tomorrow (its quite late in my timezone)

davidhewitt

Thanks! Got a few thoughts...

build.rs

pyo3-ffi/README.md

pyo3-ffi/build.rs

pyo3-ffi/src/datetime.rs

src/ffi/mod.rs

pyo3-ffi/src/lib.rs

davidhewitt

Thanks, this is shaping up nicely.

Some additional comments added. Also, some housekeeping:

The user-facing changes will need a CHANGELOG entry please.
It would be great to update Architecture.md and Contributing.md where relevant.
The coverage CI in xtask/src/main.rs will need to be updated to include this new package in coverage numbers.

davidhewitt · 2022-01-26T23:02:32Z

pyo3-ffi/src/lib.rs

+/// Thread safety is ensured by the python GIL.
+/// Therefore accessing the contents of this cell in any form while the GIL is not held is UB
+#[repr(transparent)]
+pub struct UnsafeGILCell<T>(Cell<*mut T>);


Two thoughts:

Cell just is a wrapper around UnsafeCell, so I'd prefer we use UnsafeCell as the inner type.

I don't think that we need a generic type here, given it's only used once in the datetime bindings. Also, we don't really need to give downstream users the possibility to write to the datetime pointer. Let me suggest something directly in the datetime bindings...

My Intention with the pub cell was to let downstream crates (of pyo3_ffi) use the same cell. While this is trivial to implement it would be a nice QOL addition because having mutable statics for exactly the same purpose as here (loading some python type/api capsule) is a pretty common pattern if one uses raw ffi. However I your solution below is much nicer and in the end my UnsafeGILCell is probably not gonna cover all needs of downstream crates anyway and those crates should probably just use similar wrapper types instead. It feels a little out of place in an ffi wrapper anyway

pyo3-ffi/src/datetime.rs

src/types/datetime.rs

pascalkuthe · 2022-01-26T23:36:17Z

Thanks for the feedback. I will implement your suggestions + housekeeping tomorrow.

In regards to the Changelog: I noticted that the removal of the PyDateTime_TimeZone_UTC removes the only safe way to access PyDateTime_TimeZone_UTC. While this was hidden in the ffi module and probably rarely used this was the only safe way to access PyDateTime_TimeZone_UTC with pyo3 (except for calling back into python). Should some safe replacement be provdied somewhere or ist that okay?

davidhewitt · 2022-01-26T23:44:13Z

Don't worry about PyDateTime_TimeZone_UTC - I'll make a note to myself to rebase part of #1588 after this PR lands to add that back in.

mejrs

Thanks, this looks a lot better :)

Writing these kinds of docs is not my strength so I would welcome some feedback 😄

I think they're pretty good, thanks! Don't let perfect be the enemy of good.

pyo3-ffi/README.md

pascalkuthe · 2022-01-27T00:10:57Z

I think they're pretty good, thanks! Don't let perfect be the enemy of good.

Thanks, I give it my best :) I haven't done this kind of contribution before so its just a bit of imposter syndrome.

pascalkuthe · 2022-01-28T21:31:57Z

Sorry for the delay some unexpected things came up yesterday.
I have applied the fixes as suggested.

It would be great to update Architecture.md and Contributing.md where relevant.

I found nothing relevant to update in Contributing.md . Did you have anyhing specific in mind?

davidhewitt

Thanks, this is looking great to me! @mejrs any further changes you want before this is merged?

I found nothing relevant to update in Contributing.md. Did you have anyhing specific in mind?

Nope that's fine, I was just listing off a few files which I thought would be worth checking in the light of a big structural change like this :)

I just have one tiny nit on the CHANGELOG...

CHANGELOG.md

mejrs

any further changes you want before this is merged?

LGTM :)

Thanks for doing all this work @DSPOM2

pascalkuthe · 2022-01-31T00:29:03Z

Perfect. I will apply the change to the changelog and then squash all the commits tomorrow. Then we are good to go :)
I noticted that both setup-tools-rust and maturin require patches to allow building wheels that only depend on pyo3-ffi and not on pyo3. I will try to do a PRs for that aswell but I have quite a lot on my plate so not sure how soon I will manage that

birkenfeld

Looks very good! Mostly copy-edits, a few questions.

CHANGELOG.md

Cargo.toml

pyo3-ffi/README.md

birkenfeld · 2022-01-31T05:30:50Z

src/types/datetime.rs

+    }
+}
+
+/// Type Check macros


This doesn't seem to be ta docstring for the macro?

I added this doc comment to the macro because it seemed the best place to put it (it describes the function the macro generates) but I can make it a comment instead if you think that would be clearer

Well, in the rendered docs it will show up as doc for the macro called ffi_fun_with_autoinit which seems to have nothing to do with type checking.

Okay I will put it as a comment then so its lets confusing in the docs

pyo3-ffi/src/lib.rs

pyo3-ffi/README.md

pascalkuthe · 2022-01-31T11:04:14Z

Thanks for the thorough review :)
I added 2 comments. Once we address those I will squash the commits

pascalkuthe · 2022-01-31T12:43:36Z

commits are squashed and I think this is ready to merge now now 😄

pascalkuthe · 2022-01-31T13:12:38Z

The windows coverage job keeps failing but I doesn't sound related to this PR... Not too sure what going on there

davidhewitt · 2022-01-31T23:27:22Z

Agreed. Thanks again everyone for working on this!

ijl · 2022-02-08T23:37:14Z

@DSPOM2 thanks for adding this. I have a proposal to add it to maturin in PyO3/maturin#804.

pascalkuthe mentioned this pull request Jan 25, 2022

Spin out the ffi module into its own crate #2123

Closed

pascalkuthe force-pushed the main branch from 1eafc05 to 45a0047 Compare January 25, 2022 16:50

mejrs reviewed Jan 25, 2022

View reviewed changes

pyo3-ffi/src/pyerrors.rs Outdated Show resolved Hide resolved

src/ffi/mod.rs Outdated Show resolved Hide resolved

pyo3-ffi/src/lib.rs Outdated Show resolved Hide resolved

pyo3-ffi/src/lib.rs Show resolved Hide resolved

pascalkuthe force-pushed the main branch 2 times, most recently from 1cdea7d to 23d2acc Compare January 25, 2022 19:28

pascalkuthe force-pushed the main branch 2 times, most recently from 96b4ef9 to 777eee9 Compare January 25, 2022 21:23

pascalkuthe requested a review from mejrs January 25, 2022 21:29

davidhewitt reviewed Jan 26, 2022

View reviewed changes

pascalkuthe force-pushed the main branch from 1af7949 to b447874 Compare January 26, 2022 16:00

pascalkuthe requested a review from davidhewitt January 26, 2022 18:36

adamreichold reviewed Jan 26, 2022

View reviewed changes

pyo3-ffi/src/lib.rs Outdated Show resolved Hide resolved

pascalkuthe force-pushed the main branch from b447874 to dd423a2 Compare January 26, 2022 21:36

davidhewitt reviewed Jan 26, 2022

View reviewed changes

mejrs requested changes Jan 26, 2022

View reviewed changes

pyo3-ffi/README.md Outdated Show resolved Hide resolved

pyo3-ffi/README.md Outdated Show resolved Hide resolved

pyo3-ffi/README.md Show resolved Hide resolved

pyo3-ffi/README.md Show resolved Hide resolved

pyo3-ffi/README.md Outdated Show resolved Hide resolved

mejrs mentioned this pull request Jan 27, 2022

Should ml_meth be an union? #2129

Closed

davidhewitt approved these changes Jan 30, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mejrs approved these changes Jan 31, 2022

View reviewed changes

birkenfeld requested changes Jan 31, 2022

View reviewed changes

birkenfeld approved these changes Jan 31, 2022

View reviewed changes

pascalkuthe added 2 commits January 31, 2022 13:27

move ffi module to separate crate

6a9a9ba

fix: remove excess argument for _PyCFunctionFast

004fa2a

pascalkuthe force-pushed the main branch from 425bf6c to 004fa2a Compare January 31, 2022 12:38

davidhewitt merged commit 199cc98 into PyO3:main Jan 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move ffi module to separate crate #2126

move ffi module to separate crate #2126

pascalkuthe commented Jan 25, 2022 •

edited

Loading

mejrs left a comment •

edited

Loading

davidhewitt commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022

davidhewitt commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022 •

edited

Loading

davidhewitt commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022 •

edited

Loading

pascalkuthe commented Jan 25, 2022

adamreichold commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022 •

edited

Loading

davidhewitt left a comment

davidhewitt left a comment

davidhewitt Jan 26, 2022 •

edited

Loading

pascalkuthe Jan 26, 2022 •

edited

Loading

pascalkuthe commented Jan 26, 2022

davidhewitt commented Jan 26, 2022

mejrs left a comment

pascalkuthe commented Jan 27, 2022

pascalkuthe commented Jan 28, 2022

davidhewitt left a comment

mejrs left a comment

pascalkuthe commented Jan 31, 2022

birkenfeld left a comment

birkenfeld Jan 31, 2022

pascalkuthe Jan 31, 2022

birkenfeld Jan 31, 2022

pascalkuthe Jan 31, 2022

pascalkuthe commented Jan 31, 2022

pascalkuthe commented Jan 31, 2022

pascalkuthe commented Jan 31, 2022

davidhewitt commented Jan 31, 2022

ijl commented Feb 8, 2022

move ffi module to separate crate #2126

move ffi module to separate crate #2126

Conversation

pascalkuthe commented Jan 25, 2022 • edited Loading

Open Questions

mejrs left a comment • edited Loading

Choose a reason for hiding this comment

davidhewitt commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022

davidhewitt commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022 • edited Loading

Compiled with pyo3

Compiled with pyo3-ffi

davidhewitt commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022 • edited Loading

pascalkuthe commented Jan 25, 2022

adamreichold commented Jan 25, 2022

pascalkuthe commented Jan 25, 2022 • edited Loading

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

pascalkuthe Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

pascalkuthe commented Jan 26, 2022

davidhewitt commented Jan 26, 2022

mejrs left a comment

Choose a reason for hiding this comment

pascalkuthe commented Jan 27, 2022

pascalkuthe commented Jan 28, 2022

davidhewitt left a comment

Choose a reason for hiding this comment

mejrs left a comment

Choose a reason for hiding this comment

pascalkuthe commented Jan 31, 2022

birkenfeld left a comment

Choose a reason for hiding this comment

birkenfeld Jan 31, 2022

Choose a reason for hiding this comment

pascalkuthe Jan 31, 2022

Choose a reason for hiding this comment

birkenfeld Jan 31, 2022

Choose a reason for hiding this comment

pascalkuthe Jan 31, 2022

Choose a reason for hiding this comment

pascalkuthe commented Jan 31, 2022

pascalkuthe commented Jan 31, 2022

pascalkuthe commented Jan 31, 2022

davidhewitt commented Jan 31, 2022

ijl commented Feb 8, 2022

pascalkuthe commented Jan 25, 2022 •

edited

Loading

mejrs left a comment •

edited

Loading

pascalkuthe commented Jan 25, 2022 •

edited

Loading

pascalkuthe commented Jan 25, 2022 •

edited

Loading

pascalkuthe commented Jan 25, 2022 •

edited

Loading

davidhewitt Jan 26, 2022 •

edited

Loading

pascalkuthe Jan 26, 2022 •

edited

Loading