Skip to content

Commit

Permalink
[internal] Don't clone PyAddPrefix and PyRemovePrefix (pantsbuild…
Browse files Browse the repository at this point in the history
…#13660)

Follow up to pantsbuild#13646.

The key when going from `&PyAny` to one of our custom `#[pyclass]` structs is to use `PyRef<>`, which is kind of like `RefCell<>`: https://pyo3.rs/v0.15.0/types.html#pyrefsometype-and-pyrefmutsometype

Even though this isn't a huge deal for cloning a `PathBuf`, this will become more important when porting `MergeDigests` and friends to Rust so that we don't copy the `Vec<>`.

[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano authored Nov 18, 2021
1 parent f4d63a2 commit be75c94
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 17 deletions.
4 changes: 2 additions & 2 deletions src/rust/engine/src/externs/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ impl PySnapshot {
}

#[pyclass]
#[derive(Clone, Debug, PartialEq)]
#[derive(Debug, PartialEq)]
pub struct PyAddPrefix {
pub digest: Digest,
pub prefix: PathBuf,
Expand Down Expand Up @@ -249,7 +249,7 @@ impl PyAddPrefix {
}

#[pyclass]
#[derive(Clone, Debug, PartialEq)]
#[derive(Debug, PartialEq)]
pub struct PyRemovePrefix {
pub digest: Digest,
pub prefix: PathBuf,
Expand Down
34 changes: 19 additions & 15 deletions src/rust/engine/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use futures::future::{self, BoxFuture, FutureExt, TryFutureExt};
use hashing::{Digest, EMPTY_DIGEST};
use indexmap::IndexMap;
use process_execution::{CacheDest, CacheName, NamedCaches};
use pyo3::{PyAny, Python};
use pyo3::{PyAny, PyRef, Python};
use stdio::TryCloneAsFile;
use store::{SnapshotOps, SubsetParams};
use tempfile::TempDir;
Expand Down Expand Up @@ -303,18 +303,20 @@ fn remove_prefix_request_to_digest(
args: Vec<Value>,
) -> BoxFuture<'static, NodeResult<Value>> {
async move {
let py_remove_prefix = Python::with_gil(|py| {
(*args[0])
let (digest, prefix) = Python::with_gil(|py| {
let py_remove_prefix = (*args[0])
.as_ref(py)
.extract::<PyRemovePrefix>()
.map_err(|e| throw(format!("{}", e)))
.extract::<PyRef<PyRemovePrefix>>()
.map_err(|e| throw(format!("{}", e)))?;
let prefix = RelativePath::new(&py_remove_prefix.prefix)
.map_err(|e| throw(format!("The `prefix` must be relative: {:?}", e)))?;
let res: NodeResult<(Digest, RelativePath)> = Ok((py_remove_prefix.digest, prefix));
res
})?;
let prefix = RelativePath::new(py_remove_prefix.prefix)
.map_err(|e| throw(format!("The `prefix` must be relative: {:?}", e)))?;
let digest = context
.core
.store()
.strip_prefix(py_remove_prefix.digest, prefix)
.strip_prefix(digest, prefix)
.await
.map_err(|e| throw(format!("{:?}", e)))?;
let gil = Python::acquire_gil();
Expand All @@ -328,18 +330,20 @@ fn add_prefix_request_to_digest(
args: Vec<Value>,
) -> BoxFuture<'static, NodeResult<Value>> {
async move {
let py_add_prefix = Python::with_gil(|py| {
(*args[0])
let (digest, prefix) = Python::with_gil(|py| {
let py_add_prefix = (*args[0])
.as_ref(py)
.extract::<PyAddPrefix>()
.map_err(|e| throw(format!("{}", e)))
.extract::<PyRef<PyAddPrefix>>()
.map_err(|e| throw(format!("{}", e)))?;
let prefix = RelativePath::new(&py_add_prefix.prefix)
.map_err(|e| throw(format!("The `prefix` must be relative: {:?}", e)))?;
let res: NodeResult<(Digest, RelativePath)> = Ok((py_add_prefix.digest, prefix));
res
})?;
let prefix = RelativePath::new(py_add_prefix.prefix)
.map_err(|e| throw(format!("The `prefix` must be relative: {:?}", e)))?;
let digest = context
.core
.store()
.add_prefix(py_add_prefix.digest, prefix)
.add_prefix(digest, prefix)
.await
.map_err(|e| throw(format!("{:?}", e)))?;
let gil = Python::acquire_gil();
Expand Down

0 comments on commit be75c94

Please sign in to comment.