[internal] Use PyObject instead of Value in more places (#13802)

`PyObject` (aka `Py<PyAny>`) already provides a GIL-independent reference to a Python object. Wrapping it in `Arc` is not necessary, as described at #13526 (comment).

Using `PyObject` directly removes a layer of indirection.

src/rust/engine/src/externs/engine_aware.rs

@@ -4,7 +4,6 @@
 use crate::context::Context;
 use crate::externs;
 use crate::nodes::{lift_directory_digest, lift_file_digest};
-use crate::python::Value;
 
 use crate::externs::fs::PyFileDigest;
 use pyo3::prelude::*;
@@ -121,7 +120,7 @@ fn metadata(context: &Context, obj: &PyAny) -> Option<Vec<(String, UserMetadataI
     let py_value_handle = UserMetadataPyValue::new();
     let umi = UserMetadataItem::PyValue(py_value_handle.clone());
     context.session.with_metadata_map(|map| {
-      map.insert(py_value_handle.clone(), Value::new(value.into_py(obj.py())));
+      map.insert(py_value_handle.clone(), value.into());
     });
     output.push((key, umi));
   }

src/rust/engine/src/externs/interface.rs

@@ -34,7 +34,7 @@ use pyo3::prelude::{
   PyResult as PyO3Result, Python,
 };
 use pyo3::types::{PyBytes, PyDict, PyList, PyTuple, PyType};
-use pyo3::{create_exception, PyAny};
+use pyo3::{create_exception, IntoPy, PyAny};
 use regex::Regex;
 use rule_graph::{self, RuleGraph};
 use task_executor::Executor;
@@ -360,7 +360,7 @@ impl PySession {
           core,
           should_render_ui,
           build_id,
-          session_values.into(),
+          session_values,
           cancellation_latch,
         )
       })
@@ -740,8 +740,8 @@ async fn workunit_to_py_value(
   let mut user_metadata_entries = Vec::with_capacity(workunit.metadata.user_metadata.len());
   for (user_metadata_key, user_metadata_item) in workunit.metadata.user_metadata.iter() {
     let value = match user_metadata_item {
-      UserMetadataItem::ImmediateString(v) => externs::store_utf8(py, v),
-      UserMetadataItem::ImmediateInt(n) => externs::store_i64(py, *n),
+      UserMetadataItem::ImmediateString(v) => v.into_py(py),
+      UserMetadataItem::ImmediateInt(n) => n.into_py(py),
       UserMetadataItem::PyValue(py_val_handle) => {
         match session.with_metadata_map(|map| map.get(py_val_handle).cloned()) {
           None => {
@@ -755,7 +755,10 @@ async fn workunit_to_py_value(
         }
       }
     };
-    user_metadata_entries.push((externs::store_utf8(py, user_metadata_key.as_str()), value));
+    user_metadata_entries.push((
+      externs::store_utf8(py, user_metadata_key.as_str()),
+      Value::new(value),
+    ));
   }
 
   dict_entries.push((

src/rust/engine/src/externs/mod.rs

@@ -302,7 +302,7 @@ impl Get {
       output: TypeId::new(get.product.as_ref(py)),
       input_type: TypeId::new(get.declared_subject.as_ref(py)),
       input: INTERNS
-        .key_insert(py, get.subject.clone_ref(py).into())
+        .key_insert(py, get.subject.clone_ref(py))
         .map_err(|e| Failure::from_py_err_with_gil(py, e))?,
     })
   }

src/rust/engine/src/interning.rs

@@ -6,10 +6,10 @@ use std::hash;
 use std::sync::atomic;
 
 use parking_lot::{Mutex, RwLock};
-use pyo3::prelude::{PyResult, Python};
+use pyo3::prelude::*;
 
 use crate::externs;
-use crate::python::{Fnv, Key, Value};
+use crate::python::{Fnv, Key};
 
 ///
 /// A struct that encapsulates interning of python `Value`s as comparable `Key`s.
@@ -42,7 +42,7 @@ use crate::python::{Fnv, Key, Value};
 #[derive(Default)]
 pub struct Interns {
   forward_keys: Mutex<HashMap<InternKey, Key, Fnv>>,
-  reverse_keys: RwLock<HashMap<Key, Value, Fnv>>,
+  reverse_keys: RwLock<HashMap<Key, PyObject, Fnv>>,
   id_generator: atomic::AtomicU64,
 }
 
@@ -51,10 +51,13 @@ impl Interns {
     Interns::default()
   }
 
-  pub fn key_insert(&self, py: Python, v: Value) -> PyResult<Key> {
+  pub fn key_insert(&self, py: Python, v: PyObject) -> PyResult<Key> {
     let (intern_key, type_id) = {
-      let obj = (*v).as_ref(py);
-      (InternKey(obj.hash()?, v.clone()), obj.get_type().into())
+      let obj = v.as_ref(py);
+      (
+        InternKey(obj.hash()?, v.clone_ref(py)),
+        obj.get_type().into(),
+      )
     };
 
     py.allow_threads(|| {
@@ -72,7 +75,7 @@ impl Interns {
     })
   }
 
-  pub fn key_get(&self, k: &Key) -> Value {
+  pub fn key_get(&self, k: &Key) -> PyObject {
     // NB: We do not need to acquire+release the GIL before getting a Value for a Key, because
     // neither `Key::eq` nor `Value::clone` acquire the GIL.
     self.reverse_keys.read().get(k).cloned().unwrap_or_else(|| {
@@ -93,13 +96,13 @@ impl Interns {
   }
 }
 
-struct InternKey(isize, Value);
+struct InternKey(isize, PyObject);
 
 impl Eq for InternKey {}
 
 impl PartialEq for InternKey {
   fn eq(&self, other: &InternKey) -> bool {
-    Python::with_gil(|py| externs::equals((*self.1).as_ref(py), (*other.1).as_ref(py)))
+    Python::with_gil(|py| externs::equals(self.1.as_ref(py), other.1.as_ref(py)))
   }
 }
 

src/rust/engine/src/nodes.rs

@@ -641,7 +641,7 @@ impl WrappedNode for SessionValues {
     context: Context,
     _workunit: &mut RunningWorkunit,
   ) -> NodeResult<Value> {
-    Ok(context.session.session_values())
+    Ok(Value::new(context.session.session_values()))
   }
 }
 

src/rust/engine/src/python.rs

@@ -260,11 +260,12 @@ impl Key {
 
   pub fn from_value(val: Value) -> PyResult<Key> {
     let gil = Python::acquire_gil();
-    externs::INTERNS.key_insert(gil.python(), val)
+    let py = gil.python();
+    externs::INTERNS.key_insert(py, val.consume_into_py_object(py))
   }
 
   pub fn to_value(&self) -> Value {
-    externs::INTERNS.key_get(self)
+    Value::new(externs::INTERNS.key_get(self))
   }
 }
 

src/rust/engine/src/session.rs

@@ -17,6 +17,7 @@ use futures::FutureExt;
 use graph::LastObserved;
 use log::warn;
 use parking_lot::{Mutex, RwLock};
+use pyo3::prelude::*;
 use task_executor::Executor;
 use tokio::signal::unix::{signal, SignalKind};
 use ui::ConsoleUI;
@@ -79,12 +80,12 @@ struct SessionState {
   // A place to store info about workunits in rust part
   workunit_store: WorkunitStore,
   // Per-Session values that have been set for this session.
-  session_values: Mutex<Value>,
+  session_values: Mutex<PyObject>,
   // An id used to control the visibility of uncacheable rules. Generally this is identical for an
   // entire Session, but in some cases (in particular, a `--loop`) the caller wants to retain the
   // same Session while still observing new values for uncacheable rules like Goals.
   run_id: AtomicU32,
-  workunit_metadata_map: RwLock<HashMap<UserMetadataPyValue, Value>>,
+  workunit_metadata_map: RwLock<HashMap<UserMetadataPyValue, PyObject>>,
 }
 
 ///
@@ -140,7 +141,7 @@ impl Session {
     core: Arc<Core>,
     should_render_ui: bool,
     build_id: String,
-    session_values: Value,
+    session_values: PyObject,
     cancelled: AsyncLatch,
   ) -> Result<Session, String> {
     let workunit_store = WorkunitStore::new(!should_render_ui);
@@ -226,7 +227,7 @@ impl Session {
 
   pub fn with_metadata_map<F, T>(&self, f: F) -> T
   where
-    F: FnOnce(&mut HashMap<UserMetadataPyValue, Value>) -> T,
+    F: FnOnce(&mut HashMap<UserMetadataPyValue, PyObject>) -> T,
   {
     f(&mut self.state.workunit_metadata_map.write())
   }
@@ -252,7 +253,7 @@ impl Session {
     roots.keys().map(|r| r.clone().into()).collect()
   }
 
-  pub fn session_values(&self) -> Value {
+  pub fn session_values(&self) -> PyObject {
     self.state.session_values.lock().clone()
   }