Skip to content

Commit

Permalink
Update pyo3 to 0.22
Browse files Browse the repository at this point in the history
This removes the `Clone` impl from `Py<T>` which we can work around by
keeping the GIL a bit longer in `log`. I believe that this has little to
no implication (or maybe a positive one actually due to the switch to
`clone_ref`).

However, this means that we have to remove `Clone` impl from `Logger`.

All of this is still compatible with `0.21` but still a breaking change.
  • Loading branch information
a1phyr committed Jul 1, 2024
1 parent 43a0301 commit fdd2c3b
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 19 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# 0.11.0

* Update to pyo3 0.22.

# 0.10.0

* Update to pyo3 0.21.
Expand Down
6 changes: 3 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "pyo3-log"
version = "0.10.0"
version = "0.11.0"
authors = ["Michal 'vorner' Vaner <[email protected]>"]
description = "Logging bridge from pyo3 native extension to python"
documentation = "https://docs.rs/pyo3-log"
Expand All @@ -18,10 +18,10 @@ rust-version = "1.56.0"
arc-swap = "~1"
# It's OK to ask for std on log, because pyo3 needs it too.
log = { version = "~0.4.4", default-features = false, features = ["std"] }
pyo3 = { version = ">=0.21, <0.22", default-features = false }
pyo3 = { version = ">=0.21, <0.23", default-features = false }

[dev-dependencies]
pyo3 = { version = ">=0.21, <0.22", default-features = false, features = ["auto-initialize", "macros"] }
pyo3 = { version = ">=0.21, <0.23", default-features = false, features = ["auto-initialize", "macros"] }

# `pyo3-macros` is lying about the minimal version for its `syn` dependency.
# Because we're testing with `-Zminimal-versions`, we need to explicitly set it here.
Expand Down
44 changes: 28 additions & 16 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,28 +217,45 @@ impl Default for Caching {
}
}

#[derive(Clone, Debug)]
#[derive(Debug)]
struct CacheEntry {
filter: LevelFilter,
logger: PyObject,
}

#[derive(Clone, Debug, Default)]
impl CacheEntry {
fn clone_ref(&self, py: Python<'_>) -> Self {
CacheEntry {
filter: self.filter,
logger: self.logger.clone_ref(py),
}
}
}

#[derive(Debug, Default)]
struct CacheNode {
local: Option<CacheEntry>,
children: HashMap<String, Arc<CacheNode>>,
}

impl CacheNode {
fn store_to_cache_recursive<'a, P>(&self, mut path: P, entry: CacheEntry) -> Arc<Self>
fn store_to_cache_recursive<'a, P>(
&self,
py: Python<'_>,
mut path: P,
entry: CacheEntry,
) -> Arc<Self>
where
P: Iterator<Item = &'a str>,
{
let mut me = self.clone();
let mut me = CacheNode {
children: self.children.clone(),
local: self.local.as_ref().map(|e| e.clone_ref(py)),
};
match path.next() {
Some(segment) => {
let child = me.children.entry(segment.to_owned()).or_default();
*child = child.store_to_cache_recursive(path, entry);
*child = child.store_to_cache_recursive(py, path, entry);
}
None => me.local = Some(entry),
}
Expand All @@ -253,7 +270,7 @@ impl CacheNode {
///
/// It can be either created directly and then installed, passed to other aggregating log systems,
/// or the [`init`] or [`try_init`] functions may be used if defaults are good enough.
#[derive(Clone, Debug)]
#[derive(Debug)]
pub struct Logger {
/// Filter used as a fallback if none of the `filters` match.
top_filter: LevelFilter,
Expand Down Expand Up @@ -464,12 +481,12 @@ impl Logger {
metadata.level() <= cache_filter && metadata.level() <= self.filter_for(metadata.target())
}

fn store_to_cache(&self, target: &str, entry: CacheEntry) {
fn store_to_cache(&self, py: Python<'_>, target: &str, entry: CacheEntry) {
let path = target.split("::");

let orig = self.cache.load();
// Construct a new cache structure and insert the new root.
let new = orig.store_to_cache_recursive(path, entry);
let new = orig.store_to_cache_recursive(py, path, entry);
// Note: In case of collision, the cache update is lost. This is fine, as we simply lose a
// tiny bit of performance and will cache the thing next time.
//
Expand Down Expand Up @@ -497,7 +514,6 @@ impl Log for Logger {
fn log(&self, record: &Record) {
let cache = self.lookup(record.target());

let mut store_to_cache = None;
if self.enabled_inner(record.metadata(), &cache) {
Python::with_gil(|py| match self.log_inner(py, record, &cache) {
Ok(Some(logger)) => {
Expand All @@ -510,18 +526,14 @@ impl Log for Logger {
LevelFilter::max()
}),
};
store_to_cache = Some((logger, filter));

let entry = CacheEntry { filter, logger };
self.store_to_cache(py, record.target(), entry);
}
Ok(None) => (),
Err(e) => e.print(py),
})
}
// Note: no more GIL here. Not needed for storing to cache.

if let Some((logger, filter)) = store_to_cache {
let entry = CacheEntry { filter, logger };
self.store_to_cache(record.target(), entry);
}
}

fn flush(&self) {}
Expand Down

0 comments on commit fdd2c3b

Please sign in to comment.