This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9685

copies-rust: introduce PyBytesWithData to reduce GIL requirement
ClosedPublic

Authored by SimonSapin on Jan 6 2021, 9:12 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG620c88fb42a2: copies-rust: introduce PyBytesWithData to reduce GIL requirement
rHGeef6ac04e514: copies-rust: introduce PyBytesWithData to reduce GIL requirement
Summary

See explanations in new doc-comments.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

SimonSapin created this revision.Jan 6 2021, 9:12 AM
Alphare added a subscriber: Alphare.Jan 22 2021, 5:07 AM
Alphare added inline comments.
rust/hg-cpython/src/copy_tracing.rs
29

Should that be PhantomData<PyBytes>?

SimonSapin added inline comments.Jan 22 2021, 9:05 AM
rust/hg-cpython/src/copy_tracing.rs
29

No. PhantomData<Foo> is purely type-level: it would influence the struct in terms of auto traits etc. without actually having a value of Foo.

Here we do have a value of PyBytes pointing to a specific Python object, and want to eventually call its destructor but not as long as PyBytesWithData::data can still be called.

Alphare accepted this revision.Jan 22 2021, 11:00 AM
baymax updated this revision to Diff 25705.Feb 22 2021, 9:25 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 25742.Feb 22 2021, 11:00 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 25787.Feb 22 2021, 3:47 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

SimonSapin added a commit: rHGeef6ac04e514: copies-rust: introduce PyBytesWithData to reduce GIL requirement.Feb 24 2021, 11:04 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGeef6ac04e514: copies-rust: introduce PyBytesWithData to reduce GIL requirement (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-cpython/src/copy_tracing.rs (80 lines)

Diff 25742

rust/hg-cpython/src/copy_tracing.rs

use cpython::ObjectProtocol; use cpython::ObjectProtocol;
use cpython::PyBytes; use cpython::PyBytes;
use cpython::PyDict; use cpython::PyDict;
use cpython::PyList; use cpython::PyList;
use cpython::PyModule; use cpython::PyModule;
use cpython::PyObject; use cpython::PyObject;
use cpython::PyResult; use cpython::PyResult;
use cpython::PyTuple; use cpython::PyTuple;
use cpython::Python; use cpython::Python;
use hg::copy_tracing::ChangedFiles; use hg::copy_tracing::ChangedFiles;
use hg::copy_tracing::CombineChangesetCopies; use hg::copy_tracing::CombineChangesetCopies;
use hg::Revision; use hg::Revision;
use self::pybytes_with_data::PyBytesWithData;
// Module to encapsulate private fields
mod pybytes_with_data {
use cpython::{PyBytes, Python};
/// Safe abstraction over a `PyBytes` together with the `&[u8]` slice
/// that borrows it.
///
/// Calling `PyBytes::data` requires a GIL marker but we want to access the
/// data in a thread that (ideally) does not need to acquire the GIL.
/// This type allows separating the call an the use.
pub(super) struct PyBytesWithData {
#[allow(unused)]
keep_alive: PyBytes,
AlphareUnsubmitted
Not Done

Should that be PhantomData<PyBytes>?

Alphare: Should that be `PhantomData<PyBytes>`?
SimonSapinAuthorUnsubmitted
Done

No. PhantomData<Foo> is purely type-level: it would influence the struct in terms of auto traits etc. without actually having a value of Foo.

Here we do have a value of PyBytes pointing to a specific Python object, and want to eventually call its destructor but not as long as PyBytesWithData::data can still be called.

SimonSapin: No. `PhantomData<Foo>` is purely type-level: it would influence the struct in terms of `auto`…
/// Borrows the buffer inside `self.keep_alive`,
/// but the borrow-checker cannot express self-referential structs.
data: *const [u8],
}
fn require_send<T: Send>() {}
#[allow(unused)]
fn static_assert_pybytes_is_send() {
require_send::<PyBytes>;
}
// Safety: PyBytes is Send. Raw pointers are not by default,
// but here sending one to another thread is fine since we ensure it stays
// valid.
unsafe impl Send for PyBytesWithData {}
impl PyBytesWithData {
pub fn new(py: Python, bytes: PyBytes) -> Self {
Self {
data: bytes.data(py),
keep_alive: bytes,
}
}
pub fn data(&self) -> &[u8] {
// Safety: the raw pointer is valid as long as the PyBytes is still
// alive, and the returned slice borrows `self`.
unsafe { &*self.data }
}
}
}
/// Combines copies information contained into revision `revs` to build a copy /// Combines copies information contained into revision `revs` to build a copy
/// map. /// map.
/// ///
/// See mercurial/copies.py for details /// See mercurial/copies.py for details
pub fn combine_changeset_copies_wrapper( pub fn combine_changeset_copies_wrapper(
py: Python, py: Python,
revs: PyList, revs: PyList,
children_count: PyDict, children_count: PyDict,
target_rev: Revision, target_rev: Revision,
rev_info: PyObject, rev_info: PyObject,
multi_thread: bool, multi_thread: bool,
) -> PyResult<PyDict> { ) -> PyResult<PyDict> {
let children_count = children_count let children_count = children_count
.items(py) .items(py)
.iter() .iter()
.map(|(k, v)| Ok((k.extract(py)?, v.extract(py)?))) .map(|(k, v)| Ok((k.extract(py)?, v.extract(py)?)))
.collect::<PyResult<_>>()?; .collect::<PyResult<_>>()?;
/// (Revision number, parent 1, parent 2, copy data for this revision) /// (Revision number, parent 1, parent 2, copy data for this revision)
type RevInfo = (Revision, Revision, Revision, Option<PyBytes>); type RevInfo<Bytes> = (Revision, Revision, Revision, Option<Bytes>);
let revs_info = revs.iter(py).map(|rev_py| -> PyResult<RevInfo> { let revs_info =
revs.iter(py).map(|rev_py| -> PyResult<RevInfo<PyBytes>> {
let rev = rev_py.extract(py)?; let rev = rev_py.extract(py)?;
let tuple: PyTuple = let tuple: PyTuple =
rev_info.call(py, (rev_py,), None)?.cast_into(py)?; rev_info.call(py, (rev_py,), None)?.cast_into(py)?;
let p1 = tuple.get_item(py, 0).extract(py)?; let p1 = tuple.get_item(py, 0).extract(py)?;
let p2 = tuple.get_item(py, 1).extract(py)?; let p2 = tuple.get_item(py, 1).extract(py)?;
let opt_bytes = tuple.get_item(py, 2).extract(py)?; let opt_bytes = tuple.get_item(py, 2).extract(py)?;
Ok((rev, p1, p2, opt_bytes)) Ok((rev, p1, p2, opt_bytes))
}); });
let path_copies = if !multi_thread { let path_copies = if !multi_thread {
let mut combine_changeset_copies = let mut combine_changeset_copies =
CombineChangesetCopies::new(children_count); CombineChangesetCopies::new(children_count);
for rev_info in revs_info { for rev_info in revs_info {
let (rev, p1, p2, opt_bytes) = rev_info?; let (rev, p1, p2, opt_bytes) = rev_info?;
let files = match &opt_bytes { let files = match &opt_bytes {
} else { } else {
// Use a bounded channel to provide back-pressure: // Use a bounded channel to provide back-pressure:
// if the child thread is slower to process revisions than this thread // if the child thread is slower to process revisions than this thread
// is to gather data for them, an unbounded channel would keep // is to gather data for them, an unbounded channel would keep
// growing and eat memory. // growing and eat memory.
// //
// TODO: tweak the bound? // TODO: tweak the bound?
let (rev_info_sender, rev_info_receiver) = let (rev_info_sender, rev_info_receiver) =
crossbeam_channel::bounded::<RevInfo>(1000); crossbeam_channel::bounded::<RevInfo<PyBytesWithData>>(1000);
// Start a thread that does CPU-heavy processing in parallel with the // Start a thread that does CPU-heavy processing in parallel with the
// loop below. // loop below.
// //
// If the parent thread panics, `rev_info_sender` will be dropped and // If the parent thread panics, `rev_info_sender` will be dropped and
// “disconnected”. `rev_info_receiver` will be notified of this and // “disconnected”. `rev_info_receiver` will be notified of this and
// exit its own loop. // exit its own loop.
let thread = std::thread::spawn(move || { let thread = std::thread::spawn(move || {
let mut combine_changeset_copies = let mut combine_changeset_copies =
CombineChangesetCopies::new(children_count); CombineChangesetCopies::new(children_count);
for (rev, p1, p2, opt_bytes) in rev_info_receiver { for (rev, p1, p2, opt_bytes) in rev_info_receiver {
let gil = Python::acquire_gil();
let py = gil.python();
let files = match &opt_bytes { let files = match &opt_bytes {
Some(raw) => ChangedFiles::new(raw.data(py)), Some(raw) => ChangedFiles::new(raw.data()),
// Python None was extracted to Option::None, // Python None was extracted to Option::None,
// meaning there was no copy data. // meaning there was no copy data.
None => ChangedFiles::new_empty(), None => ChangedFiles::new_empty(),
}; };
combine_changeset_copies.add_revision(rev, p1, p2, files) combine_changeset_copies.add_revision(rev, p1, p2, files)
// The GIL is (still) implicitly acquired here through
// `impl Drop for PyBytes`.
} }
combine_changeset_copies.finish(target_rev) combine_changeset_copies.finish(target_rev)
}); });
for rev_info in revs_info { for rev_info in revs_info {
let (rev, p1, p2, opt_bytes) = rev_info?; let (rev, p1, p2, opt_bytes) = rev_info?;
let opt_bytes = opt_bytes.map(|b| PyBytesWithData::new(py, b));
// We’d prefer to avoid the child thread calling into Python code, // We’d prefer to avoid the child thread calling into Python code,
// but this avoids a potential deadlock on the GIL if it does: // but this avoids a potential deadlock on the GIL if it does:
py.allow_threads(|| { py.allow_threads(|| {
rev_info_sender.send((rev, p1, p2, opt_bytes)).expect( rev_info_sender.send((rev, p1, p2, opt_bytes)).expect(
"combine_changeset_copies: channel is disconnected", "combine_changeset_copies: channel is disconnected",
); );
}); });