This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1493

pybuf: add a simple abstraction around Py_buffer interface
ClosedPublic

Authored by quark on Nov 22 2017, 8:45 PM.

Details

Reviewers
jsgf
durham
Group Reviewers
Restricted Project
Commits
rFBHGX8df65ba0169a: pybuf: add a simple abstraction around Py_buffer interface
Summary

This makes it possible to do zero-copy reading the changelog.i data from
Python to Rust because eventually changelog.i does not have to be fully
accessed and together with mmap, that's at least 100ms perf win.

There is a much more general purposed (support writable, non-contiguous
buffer) PyBuffer implementation in rust-cpython github version [1].
However, what NodeMap really wants is AsRef<[u8]> and AsRef<[u32]>,
which are not provided by that.

Since the code is short, let's just add our own wrapper. It also allows us
to use the published version of rust-cpython instead of pulling from github.

[1]: https://github.com/dgrunwald/rust-cpython/blob/master/src/buffer.rs

Test Plan

cargo build

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Nov 22 2017, 8:45 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 22 2017, 8:45 PM
quark edited the summary of this revision. (Show Details)Nov 22 2017, 9:31 PM
quark updated this revision to Diff 3793.
quark updated this revision to Diff 3796.Nov 22 2017, 9:42 PM
quark updated this revision to Diff 3799.Nov 22 2017, 10:40 PM
durham added a subscriber: durham.Dec 1 2017, 2:08 PM

Probably want Jeremy to look at this one. I'm not familiar with some of the unsafe bits.

rust/indexes/Cargo.toml
15

Should we keep the stuff with a python dependency in a separate library? To force us to not accidentally mix dependencies in the pure-rust stuff?

quark added a subscriber: mbthomas.Dec 1 2017, 4:37 PM

Maybe @mbthomas can have a look? He definitely knows (or was forced to learn about) CPython internals per the life-saving https://github.com/dgrunwald/rust-cpython/pull/115

rust/indexes/Cargo.toml
15

I think file-level separation might be good enough. The very low-level functions are already in a separate radixbuf.

jsgf requested changes to this revision.Dec 7 2017, 11:54 AM
jsgf added a subscriber: jsgf.
jsgf added inline comments.
rust/indexes/src/pybuf.rs
41

Why does this need to be #[repr(C)]? I'm not sure it really can be, technically, since PhantomData isn't really a thing in C.

47–48

I think for this to be safe, SimplePyBuf should own the GIL for its entire life. Otherwise we run the risk of the Python mutating the memory under Rust's read-only references, which is definitely undefined behaviour for Rust.

(or see below)

50

I think this should be constrained to T: Copy to make sure its a simple type - it definitely won't work if T has complex state or a Drop implementation.

76

Likewise, T: Copy

As an alternative to making SimplePyBuf always own the GIL, it could have an auxillary guard type which owns the GIL (analogous to MutexGuard), and that guard type is what implements AsRef<[T]>, so that at least while Rust is using the slice it definitely can't change.

So something like:

// assuming an existing SimplePyBuf in spb

let g = spb.lock(); // take GIL owned by SimplePyBufGuard
let s = g.as_ref(); //  get a slice view of the PyBuf
//...
mem::drop(g); // release GIL (alternatively g just goes out of scope)

SimplePyBufGuard could also implement ops::Index (index and slice) for fairly ergonomic direct access to the underlying memory.

This revision now requires changes to proceed.Dec 7 2017, 11:54 AM
quark added inline comments.Dec 7 2017, 3:12 PM
rust/indexes/src/pybuf.rs
41

Will remove. I thought an inner #[repr(C)] may be ineffective without an outer one.

47–48

Having a GIL taken is in theory more correct but practically undesirable - the SimplePyBuf object is part of a long-living struct that cannot block Python from running.

Python will make sure the python object cannot be resized so the pointer and length are always valid.

In [1]: b=bytearray([1,2,3])
In [2]: m=memoryview(b)
In [3]: b.append(4)
BufferError: Existing exports of data: object cannot be re-sized

With that, and only if the Python object is read-only, we can use the buffer confidently without gil.

Testing whether the Python object is read-only does not seem feasible for all cases. It's easy for builtin type bytes. But buffer(mmap_obj) is hard - the buffer object is read-only while its "inner" source of truth might not be. There is no way to test read-only from the inner mmap object, and no way to "unwrap" a buffer object.

50

Good idea. Will do.

76

Considering the fact that

  1. The current code is correct without gil since hg uses read-only python objects.
  2. We might migrate to a Rust-provided AsRef<[u8]> later. The lock would be unnecessary if that happens.

Since we also control the hg code path and this is not a public library, I'd prefer not adding gil for simplicity right now.

quark updated this revision to Diff 4370.Dec 11 2017, 7:20 PM
durham requested changes to this revision.Dec 14 2017, 7:32 PM
durham added inline comments.
rust/indexes/src/pybuf.rs
42

Your comment responding to Jeremy said this would be removed?

89

Jeremy mentioned using T: Copy here. You made the change above, but not here? Your response is seems to be able the GIL comment, so I just wanted to double check that the T: Copy bit wasn't lost accidentally.

This revision now requires changes to proceed.Dec 14 2017, 7:32 PM
quark added inline comments.Dec 14 2017, 7:43 PM
rust/indexes/src/pybuf.rs
42

Sorry, missed this change.

89

This is intentional. The AsRef and Drop implementation does not care about whether T is Copy or not.

quark updated this revision to Diff 4452.Dec 14 2017, 9:21 PM
durham accepted this revision.Dec 15 2017, 1:01 PM
Closed by commit rFBHGX8df65ba0169a: pybuf: add a simple abstraction around Py_buffer interface (authored by quark). · Explain WhyDec 15 2017, 2:35 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 3799

rust/indexes/Cargo.toml

[package] [package]
name = "indexes" name = "indexes"
version = "0.1.0" version = "0.1.0"
[profile.release] [profile.release]
lto = true lto = true
[lib] [lib]
name = "indexes" name = "indexes"
crate-type = ["cdylib"] crate-type = ["cdylib"]
[dependencies] [dependencies]
radixbuf = { path = "../radixbuf" } radixbuf = { path = "../radixbuf" }
error-chain = "0.11" error-chain = "0.11"
python27-sys = "*"
durhamUnsubmitted
Not Done

Should we keep the stuff with a python dependency in a separate library? To force us to not accidentally mix dependencies in the pure-rust stuff?

durham: Should we keep the stuff with a python dependency in a separate library? To force us to not…
quarkAuthorUnsubmitted
Not Done

I think file-level separation might be good enough. The very low-level functions are already in a separate radixbuf.

quark: I think file-level separation might be good enough. The very low-level functions are already in…
[dependencies.cpython]
version = "0.1.0"
default-features = false
features = ["extension-module-2-7"]

rust/indexes/src/lib.rs

// Copyright 2017 Facebook, Inc. // Copyright 2017 Facebook, Inc.
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
extern crate python27_sys;
#[macro_use]
extern crate cpython;
#[macro_use] #[macro_use]
extern crate error_chain; extern crate error_chain;
extern crate radixbuf; extern crate radixbuf;
pub mod errors; pub mod errors;
pub mod nodemap; pub mod nodemap;
mod pybuf;

rust/indexes/src/pybuf.rs

  • This file was added.
// Copyright 2017 Facebook, Inc.
//
// This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version.
//! A simple `Py_buffer` wrapper that allows zero-copy reading of Python
//! owned memory.
// The objects in memory have a relationship like:
//
// ```text
// SimplePyBuf<T> | Raw Data Python object
// +-----------+ | +-------+ +-----------+
// | Py_buffer | | | | <-- owns -- _ |
// | +-------+ | | | | +-----------+
// | | *buf -- points to --> | |
// | | len | | | | |
// | +-------+ | | +-------+
// +-----------+ |
// |
// Rust-managed | Python-managed
// ```
//
// Notes:
// - Raw data is owned by (embedded in, or pointed by) the Python object.
// Raw data gets freed when the Python object is destructed.
// - Py_buffer is not a Python object but a Python-defined C struct allowing
// native code to access "Raw data" directly. When constructing Py_buffer
// from a Python object, the refcount of that Python object increases.
// The refcount decreases when Py_buffer gets destructed via PyBuffer_Release.
// - Py_buffer is used to expose the raw pointer and length.
// - Memory alignment is up to the actual implementation of "Python object".
// For a mmap buffer, the libc mmap function guarantees that.
use std::marker::PhantomData;
use std::mem;
use std::slice;
use cpython::{Python, PyObject};
use python27_sys as cpy;
#[repr(C)]
jsgfUnsubmitted
Not Done

Why does this need to be #[repr(C)]? I'm not sure it really can be, technically, since PhantomData isn't really a thing in C.

jsgf: Why does this need to be `#[repr(C)]`? I'm not sure it really can be, technically, since…
quarkAuthorUnsubmitted
Not Done

Will remove. I thought an inner #[repr(C)] may be ineffective without an outer one.

quark: Will remove. I thought an inner `#[repr(C)]` may be ineffective without an outer one.
pub struct SimplePyBuf<T>(cpy::Py_buffer, PhantomData<T>);
durhamUnsubmitted
Not Done

Your comment responding to Jeremy said this would be removed?

durham: Your comment responding to Jeremy said this would be removed?
quarkAuthorUnsubmitted
Not Done

Sorry, missed this change.

quark: Sorry, missed this change.
// Since the buffer is read-only and Python cannot move the raw buffer (because
// we own the Py_buffer struct). It's safe to share and use SimplePyBuf in other
// threads.
unsafe impl<T> Send for SimplePyBuf<T> {}
unsafe impl<T> Sync for SimplePyBuf<T> {}
jsgfUnsubmitted
Not Done

I think for this to be safe, SimplePyBuf should own the GIL for its entire life. Otherwise we run the risk of the Python mutating the memory under Rust's read-only references, which is definitely undefined behaviour for Rust.

(or see below)

jsgf: I think for this to be safe, `SimplePyBuf` should own the GIL for its entire life. Otherwise we…
quarkAuthorUnsubmitted
Not Done

Having a GIL taken is in theory more correct but practically undesirable - the SimplePyBuf object is part of a long-living struct that cannot block Python from running.

Python will make sure the python object cannot be resized so the pointer and length are always valid.

In [1]: b=bytearray([1,2,3])
In [2]: m=memoryview(b)
In [3]: b.append(4)
BufferError: Existing exports of data: object cannot be re-sized

With that, and only if the Python object is read-only, we can use the buffer confidently without gil.

Testing whether the Python object is read-only does not seem feasible for all cases. It's easy for builtin type bytes. But buffer(mmap_obj) is hard - the buffer object is read-only while its "inner" source of truth might not be. There is no way to test read-only from the inner mmap object, and no way to "unwrap" a buffer object.

quark: Having a GIL taken is in theory more correct but practically undesirable - the `SimplePyBuf`…
impl<T> SimplePyBuf<T> {
jsgfUnsubmitted
Not Done

I think this should be constrained to T: Copy to make sure its a simple type - it definitely won't work if T has complex state or a Drop implementation.

jsgf: I think this should be constrained to `T: Copy` to make sure its a simple type - it definitely…
quarkAuthorUnsubmitted
Not Done

Good idea. Will do.

quark: Good idea. Will do.
pub fn new(_py: Python, obj: &PyObject) -> Self {
// Note about GC on obj:
//
// Practically, obj here is some low-level, non-container ones like
// bytes or memoryview that does not support GC (i.e. do not have
// Py_TPFLAGS_HAVE_GC set). refcount is the only way to release them.
// So no need to pay extra attention on them - SimplePyBuf will get
// refcount right and that's enough.
//
// Otherwise (obj is a container type that does support GC), whoever
// owns this SimplePyBuf in the Rust world needs to do one of the
// following:
// - implement tp_traverse in its Python class
// - call PyObject_GC_UnTrack to let GC ignore obj
unsafe {
let mut buf = mem::zeroed::<SimplePyBuf<T>>();
let r = cpy::PyObject_GetBuffer(obj.as_ptr(), &mut buf.0, cpy::PyBUF_SIMPLE);
if r == -1 {
panic!("failed to get Py_buffer");
}
buf
}
}
}
impl<T> AsRef<[T]> for SimplePyBuf<T> {
jsgfUnsubmitted
Not Done

Likewise, T: Copy

As an alternative to making SimplePyBuf always own the GIL, it could have an auxillary guard type which owns the GIL (analogous to MutexGuard), and that guard type is what implements AsRef<[T]>, so that at least while Rust is using the slice it definitely can't change.

So something like:

// assuming an existing SimplePyBuf in spb

let g = spb.lock(); // take GIL owned by SimplePyBufGuard
let s = g.as_ref(); //  get a slice view of the PyBuf
//...
mem::drop(g); // release GIL (alternatively g just goes out of scope)

SimplePyBufGuard could also implement ops::Index (index and slice) for fairly ergonomic direct access to the underlying memory.

jsgf: Likewise, `T: Copy` As an alternative to making `SimplePyBuf` always own the GIL, it could…
quarkAuthorUnsubmitted
Not Done

Considering the fact that

  1. The current code is correct without gil since hg uses read-only python objects.
  2. We might migrate to a Rust-provided AsRef<[u8]> later. The lock would be unnecessary if that happens.

Since we also control the hg code path and this is not a public library, I'd prefer not adding gil for simplicity right now.

quark: Considering the fact that 1. The current code is correct without gil since hg uses read-only…
#[inline]
fn as_ref(&self) -> &[T] {
let len = self.0.len as usize / mem::size_of::<T>();
unsafe { slice::from_raw_parts(self.0.buf as *const T, len) }
}
}
impl<T> Drop for SimplePyBuf<T> {
fn drop(&mut self) {
let _gil = Python::acquire_gil();
unsafe { cpy::PyBuffer_Release(&mut self.0) }
}
}
durhamUnsubmitted
Not Done

Jeremy mentioned using T: Copy here. You made the change above, but not here? Your response is seems to be able the GIL comment, so I just wanted to double check that the T: Copy bit wasn't lost accidentally.

durham: Jeremy mentioned using `T: Copy` here. You made the change above, but not here? Your response…
quarkAuthorUnsubmitted
Not Done

This is intentional. The AsRef and Drop implementation does not care about whether T is Copy or not.

quark: This is intentional. The `AsRef` and `Drop` implementation does not care about whether T is…