From f9528b4ecd1b0c260df7e8ad57b9ddc4da09d273 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Fri, 10 Nov 2023 14:17:42 -0500 Subject: [PATCH] pep440-rs: switch Version::release to smallvec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit attempts an optimization that switches a version's `release` field over to a `smallvec` optimization. The idea is that most versions are very small and can be stored inline. Interestingly, I was unable to observe any obvious benefit: $ hyperfine \ "./target/profiling/puffin-dev-u32 resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null" \ "./target/profiling/puffin-dev-smallvec-release resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null" Benchmark 1: ./target/profiling/puffin-dev-u32 resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null Time (mean ± σ): 872.2 ms ± 26.5 ms [User: 14646.0 ms, System: 2516.0 ms] Range (min … max): 833.0 ms … 912.0 ms 10 runs Benchmark 2: ./target/profiling/puffin-dev-smallvec-release resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null Time (mean ± σ): 882.3 ms ± 17.4 ms [User: 14764.4 ms, System: 2520.9 ms] Range (min … max): 859.7 ms … 912.7 ms 10 runs Summary './target/profiling/puffin-dev-u32 resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null' ran 1.01 ± 0.04 times faster than './target/profiling/puffin-dev-smallvec-release resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null' My hypothesis is that because of an earlier commit that switched the global allocator to jemalloc, the cost of allocation had precipitously decreased. To the point that the reduction in allocs from the smallvec becomes a wash. To test my hypothesis, I dropped the jemalloc commit and measured the perf of the smallvec optimization against main: $ hyperfine \ "./target/profiling/puffin-dev-main resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null" \ "./target/profiling/puffin-dev-smallvec-release-no-jemalloc resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null" Benchmark 1: ./target/profiling/puffin-dev-main resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null Time (mean ± σ): 968.0 ms ± 20.0 ms [User: 17637.4 ms, System: 2151.9 ms] Range (min … max): 940.2 ms … 1005.3 ms 10 runs Benchmark 2: ./target/profiling/puffin-dev-smallvec-release-no-jemalloc resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null Time (mean ± σ): 958.4 ms ± 15.7 ms [User: 17119.7 ms, System: 2246.1 ms] Range (min … max): 944.7 ms … 993.3 ms 10 runs Summary './target/profiling/puffin-dev-smallvec-release-no-jemalloc resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null' ran 1.01 ± 0.03 times faster than './target/profiling/puffin-dev-main resolve-many --cache-dir cache-docker-no-build --no-build pypi_top_8k_flat.txt --limit 1000 2> /dev/null' Fiddlesticks. Even when allocation is (presumably) more expensive, the smallvec optimization didn't help. This suggests something is off about my mental model of the code. So there are more avenues to explore here! --- Cargo.lock | 6 ++++-- Cargo.toml | 1 + crates/pep440-rs/Cargo.toml | 1 + crates/pep440-rs/src/version.rs | 9 +++++---- crates/pep440-rs/src/version_specifier.rs | 9 +++++---- crates/pep508-rs/Cargo.toml | 1 + crates/pep508-rs/src/lib.rs | 5 +++-- 7 files changed, 20 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ab2f6dc5f..800bc93a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2034,6 +2034,7 @@ dependencies = [ "pyo3", "regex", "serde", + "smallvec", "tracing", "unicode-width", ] @@ -2064,6 +2065,7 @@ dependencies = [ "regex", "serde", "serde_json", + "smallvec", "testing_logger", "thiserror", "tracing", @@ -3275,9 +3277,9 @@ dependencies = [ [[package]] name = "smallvec" -version = "1.11.1" +version = "1.11.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "942b4a808e05215192e39f4ab80813e599068285906cc91aa64f923db842bd5a" +checksum = "4dccd0940a2dcdf68d092b8cbab7dc0ad8fa938bf95787e1b916b0e3d0e8e970" [[package]] name = "smawk" diff --git a/Cargo.toml b/Cargo.toml index b0b1eb6a8..500450e34 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,6 +60,7 @@ seahash = { version = "4.1.0" } serde = { version = "1.0.190" } serde_json = { version = "1.0.108" } sha2 = { version = "0.10.8" } +smallvec = { version = "1.11.2" } tar = { version = "0.4.40" } target-lexicon = { version = "0.12.12" } tempfile = { version = "3.8.1" } diff --git a/crates/pep440-rs/Cargo.toml b/crates/pep440-rs/Cargo.toml index 4be33a51a..2029c1a6b 100644 --- a/crates/pep440-rs/Cargo.toml +++ b/crates/pep440-rs/Cargo.toml @@ -20,6 +20,7 @@ crate-type = ["rlib", "cdylib"] once_cell = { workspace = true } regex = { workspace = true } serde = { workspace = true, features = ["derive"], optional = true } +smallvec = { workspace = true } tracing = { workspace = true, optional = true } unicode-width = { workspace = true } diff --git a/crates/pep440-rs/src/version.rs b/crates/pep440-rs/src/version.rs index f2b0dd145..c39fe8393 100644 --- a/crates/pep440-rs/src/version.rs +++ b/crates/pep440-rs/src/version.rs @@ -8,6 +8,7 @@ use regex::Captures; use regex::Regex; #[cfg(feature = "serde")] use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; +use smallvec::SmallVec; use std::cmp::{max, Ordering}; #[cfg(feature = "pyo3")] use std::collections::hash_map::DefaultHasher; @@ -269,7 +270,7 @@ pub struct Version { /// such a `1.2.3` in `4!1.2.3-a8.post9.dev1` /// /// Note that we drop the * placeholder by moving it to `Operator` - pub release: Vec, + pub release: SmallVec<[u32; 8]>, /// The [prerelease](https://peps.python.org/pep-0440/#pre-releases), i.e. alpha, beta or rc /// plus a number /// @@ -328,7 +329,7 @@ impl PyVersion { /// Note that we drop the * placeholder by moving it to `Operator` #[getter] pub fn release(&self) -> Vec { - self.0.release.clone() + self.0.release.to_vec() } /// The [prerelease](https://peps.python.org/pep-0440/#pre-releases), i.e. alpha, beta or rc /// plus a number @@ -445,7 +446,7 @@ impl Version { pub fn from_release(release: Vec) -> Self { Self { epoch: 0, - release, + release: SmallVec::from(release), pre: None, post: None, dev: None, @@ -785,7 +786,7 @@ impl Version { .as_str() .split('.') .map(|segment| segment.parse::().map_err(|err| err.to_string())) - .collect::, String>>()?; + .collect::, String>>()?; let star = captures.name("trailing_dot_star").is_some(); if star { diff --git a/crates/pep440-rs/src/version_specifier.rs b/crates/pep440-rs/src/version_specifier.rs index e4f96b298..1a3efdc2b 100644 --- a/crates/pep440-rs/src/version_specifier.rs +++ b/crates/pep440-rs/src/version_specifier.rs @@ -566,6 +566,7 @@ pub fn parse_version_specifiers(spec: &str) -> Result, Pep mod test { use crate::{Operator, Version, VersionSpecifier, VersionSpecifiers}; use indoc::indoc; + use smallvec::smallvec; use std::cmp::Ordering; use std::str::FromStr; @@ -1060,7 +1061,7 @@ mod test { operator: Operator::TildeEqual, version: Version { epoch: 0, - release: vec![0, 9], + release: smallvec![0, 9], pre: None, post: None, dev: None, @@ -1071,7 +1072,7 @@ mod test { operator: Operator::GreaterThanEqual, version: Version { epoch: 0, - release: vec![1, 0], + release: smallvec![1, 0], pre: None, post: None, dev: None, @@ -1082,7 +1083,7 @@ mod test { operator: Operator::NotEqualStar, version: Version { epoch: 0, - release: vec![1, 3, 4], + release: smallvec![1, 3, 4], pre: None, post: None, dev: None, @@ -1093,7 +1094,7 @@ mod test { operator: Operator::LessThan, version: Version { epoch: 0, - release: vec![2, 0], + release: smallvec![2, 0], pre: None, post: None, dev: None, diff --git a/crates/pep508-rs/Cargo.toml b/crates/pep508-rs/Cargo.toml index 0c68e169e..f6371fc27 100644 --- a/crates/pep508-rs/Cargo.toml +++ b/crates/pep508-rs/Cargo.toml @@ -37,6 +37,7 @@ indoc = "2.0.4" log = "0.4.20" testing_logger = "0.1.1" serde_json = "1.0.108" +smallvec = { workspace = true } [features] pyo3 = ["dep:pyo3", "pep440_rs/pyo3", "pyo3-log"] diff --git a/crates/pep508-rs/src/lib.rs b/crates/pep508-rs/src/lib.rs index 9b9114cc0..d4a77abf0 100644 --- a/crates/pep508-rs/src/lib.rs +++ b/crates/pep508-rs/src/lib.rs @@ -888,6 +888,7 @@ mod tests { use std::str::FromStr; use indoc::indoc; + use smallvec::smallvec; use url::Url; use pep440_rs::{Operator, Version, VersionSpecifier}; @@ -956,7 +957,7 @@ mod tests { Operator::GreaterThanEqual, Version { epoch: 0, - release: vec![2, 8, 1], + release: smallvec![2, 8, 1], pre: None, post: None, dev: None, @@ -969,7 +970,7 @@ mod tests { Operator::Equal, Version { epoch: 0, - release: vec![2, 8], + release: smallvec![2, 8], pre: None, post: None, dev: None,