From f0ad6b5da3d2b658d4700fab0abb1602f9741721 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Thu, 9 Jan 2025 10:14:14 +0100 Subject: [PATCH 1/3] Remove largely useless string interning support --- Makefile | 2 +- minijinja/Cargo.toml | 4 +- minijinja/src/lib.rs | 6 --- minijinja/src/macros.rs | 7 ---- minijinja/src/template.rs | 5 +-- minijinja/src/value/mod.rs | 51 ++----------------------- minijinja/src/value/object.rs | 20 +++------- minijinja/src/value/string_interning.rs | 47 ----------------------- minijinja/src/vm/mod.rs | 5 +-- 9 files changed, 14 insertions(+), 133 deletions(-) delete mode 100644 minijinja/src/value/string_interning.rs diff --git a/Makefile b/Makefile index 301ef333..7eac2033 100644 --- a/Makefile +++ b/Makefile @@ -15,7 +15,7 @@ doc: .PHONY: test-msrv test-msrv: @$(MAKE) run-tests FEATURES=$(TEST_FEATURES) - @$(MAKE) run-tests FEATURES=$(TEST_FEATURES),preserve_order,key_interning,unicode + @$(MAKE) run-tests FEATURES=$(TEST_FEATURES),preserve_order,unicode @echo "CARGO TEST ALL FEATURES" @cd minijinja; cargo test --all-features diff --git a/minijinja/Cargo.toml b/minijinja/Cargo.toml index 325da662..c1ca1596 100644 --- a/minijinja/Cargo.toml +++ b/minijinja/Cargo.toml @@ -39,7 +39,6 @@ std_collections = [] serde = [] # Speedups -key_interning = [] speedups = ["v_htmlescape"] # Engine Features @@ -54,6 +53,9 @@ fuel = [] json = ["serde_json"] urlencode = ["percent-encoding"] +# Deprecated features +key_interning = [] + # Internal Features that should not be used internal_debug = [] unstable_machinery = ["internal_debug"] diff --git a/minijinja/src/lib.rs b/minijinja/src/lib.rs index 0e707bb9..febae846 100644 --- a/minijinja/src/lib.rs +++ b/minijinja/src/lib.rs @@ -187,12 +187,6 @@ //! limited. //! - `speedups`: enables all speedups, in particular it turns on the `v_htmlescape` dependency //! for faster HTML escaping. -//! - `key_interning`: if this feature is enabled the automatic string interning in -//! the value type is enabled. This feature used to be turned on by default but -//! has negative performance effects in newer versions of MiniJinja since a lot of -//! the previous uses of key interning are no longer needed. Enabling it however -//! cuts down on memory usage slightly in certain scenarios by interning all string -//! keys used in dynamic map values. //! //! Internals: //! diff --git a/minijinja/src/macros.rs b/minijinja/src/macros.rs index ba37c90d..9111d6ad 100644 --- a/minijinja/src/macros.rs +++ b/minijinja/src/macros.rs @@ -28,11 +28,6 @@ pub mod __context { use crate::Environment; use std::rc::Rc; - #[inline(always)] - pub fn value_optimization() -> impl Drop { - crate::value::value_optimization() - } - #[inline(always)] pub fn make() -> ValueMap { ValueMap::default() @@ -137,7 +132,6 @@ macro_rules! context { $($key:ident $(=> $value:expr)?),* $(, .. $ctx:expr),* $(,)? ) => {{ - let _guard = $crate::__context::value_optimization(); let mut ctx = $crate::__context::make(); $( $crate::__context_pair!(ctx, $key $(=> $value)?); @@ -157,7 +151,6 @@ macro_rules! context { ( $(.. $ctx:expr),* $(,)? ) => {{ - let _guard = $crate::__context::value_optimization(); let mut ctx = ::std::vec::Vec::new(); $( ctx.push($crate::value::Value::from($ctx)); diff --git a/minijinja/src/template.rs b/minijinja/src/template.rs index 37496533..6400988e 100644 --- a/minijinja/src/template.rs +++ b/minijinja/src/template.rs @@ -15,7 +15,7 @@ use crate::error::{attach_basic_debug_info, Error}; use crate::output::{Output, WriteWrapper}; use crate::syntax::SyntaxConfig; use crate::utils::AutoEscape; -use crate::value::{self, Value}; +use crate::value::Value; use crate::vm::{prepare_blocks, Context, State, Vm}; /// Callback for auto escape determination @@ -372,9 +372,6 @@ impl<'source> CompiledTemplate<'source> { source: &'source str, config: &TemplateConfig, ) -> Result, Error> { - // the parser/compiler combination can create constants in which case - // we can probably benefit from the value optimization a bit. - let _guard = value::value_optimization(); let ast = ok!(parse( source, name, diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index f14ba26f..dc41a7c5 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -229,8 +229,6 @@ pub(crate) mod namespace_object; mod object; pub(crate) mod ops; mod serialize; -#[cfg(feature = "key_interning")] -mod string_interning; #[cfg(feature = "deserialization")] pub use self::deserialize::ViaDeserialize; @@ -287,22 +285,6 @@ pub fn serializing_for_value() -> bool { INTERNAL_SERIALIZATION.with(|flag| flag.get()) } -/// Enables value optimizations. -/// -/// If `key_interning` is enabled, this turns on that feature, otherwise -/// it becomes a noop. -#[inline(always)] -pub(crate) fn value_optimization() -> impl Drop { - #[cfg(feature = "key_interning")] - { - crate::value::string_interning::use_string_cache() - } - #[cfg(not(feature = "key_interning"))] - { - OnDrop::new(|| {}) - } -} - fn mark_internal_serialization() -> impl Drop { let old = INTERNAL_SERIALIZATION.with(|flag| { let old = flag.get(); @@ -685,36 +667,10 @@ impl Default for Value { } } -/// Intern a string. -/// -/// When the `key_interning` feature is in used, then MiniJinja will attempt to -/// reuse strings in certain cases. This function can be used to utilize the -/// same functionality. There is no guarantee that a string will be interned -/// as there are heuristics involved for it. Additionally the string interning -/// will only work during the template engine execution (eg: within filters etc.). -/// -/// The use of this function is generally recommended against and it might -/// become deprecated in the future. +#[doc(hidden)] +#[deprecated = "This function no longer has an effect. Use Arc::from directly."] pub fn intern(s: &str) -> Arc { - #[cfg(feature = "key_interning")] - { - crate::value::string_interning::try_intern(s) - } - #[cfg(not(feature = "key_interning"))] - { - Arc::from(s.to_string()) - } -} - -/// Like [`intern`] but returns a [`Value`] instead of an `Arc`. -/// -/// This has the benefit that it will only perform interning if the string -/// is not already interned. -pub(crate) fn intern_into_value(s: &str) -> Value { - match SmallStr::try_new(s) { - Some(small_str) => Value(ValueRepr::SmallStr(small_str)), - None => Value::from(intern(s)), - } + Arc::from(s.to_string()) } #[allow(clippy::len_without_is_empty)] @@ -755,7 +711,6 @@ impl Value { /// serde deserialization. pub fn from_serialize(value: T) -> Value { let _serialization_guard = mark_internal_serialization(); - let _optimization_guard = value_optimization(); transform(value) } diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index 00e1c044..1bcc654c 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -5,7 +5,7 @@ use std::hash::Hash; use std::sync::Arc; use crate::error::{Error, ErrorKind}; -use crate::value::{intern, intern_into_value, mapped_enumerator, Value}; +use crate::value::{mapped_enumerator, Value}; use crate::vm::State; /// A trait that represents a dynamic object. @@ -297,7 +297,7 @@ macro_rules! impl_object_helpers { } Enumerator::Iter(iter) => Some(iter), Enumerator::RevIter(iter) => Some(Box::new(iter)), - Enumerator::Str(s) => Some(Box::new(s.iter().copied().map(intern_into_value))), + Enumerator::Str(s) => Some(Box::new(s.iter().copied().map(Value::from))), Enumerator::Values(v) => Some(Box::new(v.into_iter())), } } @@ -736,9 +736,7 @@ macro_rules! impl_str_map_helper { } fn enumerate(self: &Arc) -> Enumerator { - self.$enumerator(|this| { - Box::new(this.keys().map(|x| intern_into_value(x.as_ref()))) - }) + self.$enumerator(|this| Box::new(this.keys().map(|x| Value::from(x as &str)))) } fn enumerator_len(self: &Arc) -> Option { @@ -778,7 +776,7 @@ macro_rules! impl_str_map { fn from(val: $map_type<&'a str, V>) -> Self { Value::from( val.into_iter() - .map(|(k, v)| (intern(k), v)) + .map(|(k, v)| (Arc::from(k), v)) .collect::<$map_type, V>>(), ) } @@ -791,15 +789,7 @@ macro_rules! impl_str_map { fn from(val: $map_type, V>) -> Self { Value::from( val.into_iter() - .map(|(k, v)| { - ( - match k { - Cow::Borrowed(s) => intern(s), - Cow::Owned(s) => Arc::::from(s), - }, - v, - ) - }) + .map(|(k, v)| (Arc::from(k), v)) .collect::<$map_type, V>>(), ) } diff --git a/minijinja/src/value/string_interning.rs b/minijinja/src/value/string_interning.rs deleted file mode 100644 index 3e58c05b..00000000 --- a/minijinja/src/value/string_interning.rs +++ /dev/null @@ -1,47 +0,0 @@ -/// Utility module to help with string interning. -use crate::utils::OnDrop; - -use std::cell::{Cell, RefCell}; -use std::collections::HashSet; -use std::sync::Arc; - -thread_local! { - static STRING_KEY_CACHE: RefCell>> = Default::default(); - static USE_STRING_KEY_CACHE: Cell = const { Cell::new(false) }; -} - -pub(crate) fn use_string_cache() -> impl Drop { - let was_enabled = USE_STRING_KEY_CACHE.with(|flag| { - let was_enabled = flag.get(); - flag.set(true); - was_enabled - }); - OnDrop::new(move || { - if !was_enabled { - USE_STRING_KEY_CACHE.with(|flag| flag.set(false)); - STRING_KEY_CACHE.with(|cache| cache.borrow_mut().clear()); - } - }) -} - -#[inline(always)] -pub(crate) fn try_intern(s: &str) -> Arc { - // strings longer than 16 bytes are never interned or if we're at - // depth 0. (serialization code outside of internal serialization) - // not checking for depth can cause a memory leak. - if s.len() > 16 || !USE_STRING_KEY_CACHE.with(|flag| flag.get()) { - return Arc::from(s); - } - - STRING_KEY_CACHE.with(|cache| { - let mut set = cache.borrow_mut(); - match set.get(s) { - Some(stored) => stored.clone(), - None => { - let rv: Arc = Arc::from(s.to_string()); - set.insert(rv.clone()); - rv - } - } - }) -} diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index 4119b3ff..4ecb90e1 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -11,9 +11,7 @@ use crate::error::{Error, ErrorKind}; use crate::output::{CaptureMode, Output}; use crate::utils::{untrusted_size_hint, AutoEscape, UndefinedBehavior}; use crate::value::namespace_object::Namespace; -use crate::value::{ - ops, value_map_with_capacity, value_optimization, Kwargs, ObjectRepr, Value, ValueMap, -}; +use crate::value::{ops, value_map_with_capacity, Kwargs, ObjectRepr, Value, ValueMap}; use crate::vm::context::{Frame, LoopState, Stack}; use crate::vm::loop_object::Loop; use crate::vm::state::BlockStack; @@ -91,7 +89,6 @@ impl<'env> Vm<'env> { out: &mut Output, auto_escape: AutoEscape, ) -> Result<(Option, State<'template, 'env>), Error> { - let _guard = value_optimization(); let mut state = State::new( self.env, Context::new_with_frame(ok!(Frame::new_checked(root)), self.env.recursion_limit()), From 0e1ae1568a899cdc13b22615ab84e526fcc90dd0 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Thu, 9 Jan 2025 10:15:50 +0100 Subject: [PATCH 2/3] Add changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36dbdb03..a3d29a61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ All notable changes to MiniJinja are documented here. +## 2.7.0 + +- Removed string interning. #675 + ## 2.6.0 - Added `sum` filter. #648 From 561af1a921866c965e7d9f48b35e1d6bf56b7721 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Thu, 9 Jan 2025 12:51:20 +0100 Subject: [PATCH 3/3] Fix typo --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 097900f2..18c24312 100644 --- a/README.md +++ b/README.md @@ -90,7 +90,7 @@ the engine is used so you can see how it's utilized: * **[Oranda](https://github.com/axodotdev/oranda)** uses it to [generate HTML landing pages](https://github.com/axodotdev/oranda/blob/fb97859c99ab81f644ab5b1449f725fc5c3e9721/src/site/templates.rs) * Code Generation: - * **[OpenTelemetry's Weaver](https://github.com/open-telemetry/weaver)** uses it to [generate documentation, code and other outputs](https://github.com/open-telemetry/weaver/blob/d49881445e09beb42e1a394bfa5f3068c660daf3/crates/weaver_forge/src/lib.rs#L482-L567) from the OTel specificiation. + * **[OpenTelemetry's Weaver](https://github.com/open-telemetry/weaver)** uses it to [generate documentation, code and other outputs](https://github.com/open-telemetry/weaver/blob/d49881445e09beb42e1a394bfa5f3068c660daf3/crates/weaver_forge/src/lib.rs#L482-L567) from the OTel specification. * Structure Generation: * **[Astral's Rye](https://rye.astral.sh/)** is using it to [generate project structures](https://github.com/astral-sh/rye/blob/c60682fb6bb5c9a0cc2669f263eeed99d2e5be71/rye/src/cli/init.rs)