From b3fed83bd953fea88ce7f1740347da44944ccec9 Mon Sep 17 00:00:00 2001 From: peefy Date: Fri, 24 May 2024 10:26:18 +0800 Subject: [PATCH] fix: runtime option function memory leak and add gc function calling in the evluator Signed-off-by: peefy --- kclvm/api/Cargo.toml | 2 +- kclvm/runner/src/runner.rs | 3 ++ kclvm/runtime/src/api/kclvm.rs | 3 +- kclvm/runtime/src/context/mod.rs | 11 ++++- kclvm/runtime/src/stdlib/builtin.rs | 14 ++----- kclvm/runtime/src/stdlib/builtin_api.rs | 24 ++++------- kclvm/runtime/src/value/iter.rs | 55 +++++++------------------ 7 files changed, 42 insertions(+), 70 deletions(-) diff --git a/kclvm/api/Cargo.toml b/kclvm/api/Cargo.toml index 2071c3546..5a5e0a213 100644 --- a/kclvm/api/Cargo.toml +++ b/kclvm/api/Cargo.toml @@ -41,7 +41,7 @@ tokio = { version = "1.37.0", features = ["full"] } [dev-dependencies] criterion = "0.4.0" -[build_dependencies] +[build-dependencies] protoc-bin-vendored = "3.0.0" prost-build = "0.11.8" prost-wkt-build = {path = "../third-party/prost-wkt/wkt-build", version = "0.4.1"} diff --git a/kclvm/runner/src/runner.rs b/kclvm/runner/src/runner.rs index f06fc28d9..16c7979c7 100644 --- a/kclvm/runner/src/runner.rs +++ b/kclvm/runner/src/runner.rs @@ -557,6 +557,9 @@ impl FastRunner { Err(err) => err.to_string(), }; } + // Free all value references at runtime. This is because the runtime context marks + // all KCL objects and holds their copies, so it is necessary to actively GC them. + ctx.borrow().gc(); Ok(result) } } diff --git a/kclvm/runtime/src/api/kclvm.rs b/kclvm/runtime/src/api/kclvm.rs index fdba18c6a..773014a34 100644 --- a/kclvm/runtime/src/api/kclvm.rs +++ b/kclvm/runtime/src/api/kclvm.rs @@ -342,7 +342,7 @@ pub struct Context { /// Imported package path to check the cyclic import process. pub imported_pkgpath: HashSet, /// Runtime arguments for the option function. - pub app_args: HashMap, + pub option_values: HashMap, /// All schema instances, the first key is the schema runtime type and /// the second key is the schema instance package path pub instances: IndexMap>>, @@ -350,7 +350,6 @@ pub struct Context { pub all_schemas: HashMap, /// Import graph pub import_names: IndexMap>, - /// A buffer to store plugin or hooks function calling results. pub buffer: ContextBuffer, /// Objects is to store all KCL object pointers at runtime. diff --git a/kclvm/runtime/src/context/mod.rs b/kclvm/runtime/src/context/mod.rs index 0080b69a9..ec24cc733 100644 --- a/kclvm/runtime/src/context/mod.rs +++ b/kclvm/runtime/src/context/mod.rs @@ -4,7 +4,7 @@ pub mod api; pub use api::*; use std::fmt; -use crate::{BacktraceFrame, PanicInfo, RuntimePanicRecord}; +use crate::{kclvm_value_delete, kclvm_value_ref_t, BacktraceFrame, PanicInfo, RuntimePanicRecord}; impl fmt::Display for PanicInfo { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { @@ -179,4 +179,13 @@ impl crate::Context { self.panic_info.rust_line = record.rust_line; self.panic_info.rust_col = record.rust_col; } + + pub fn gc(&self) { + unsafe { + for o in &self.objects { + let ptr = (*o) as *mut kclvm_value_ref_t; + kclvm_value_delete(ptr); + } + } + } } diff --git a/kclvm/runtime/src/stdlib/builtin.rs b/kclvm/runtime/src/stdlib/builtin.rs index cd9f70e1d..901a4bd87 100644 --- a/kclvm/runtime/src/stdlib/builtin.rs +++ b/kclvm/runtime/src/stdlib/builtin.rs @@ -7,21 +7,15 @@ use crate::*; impl Context { pub fn builtin_option_init(&mut self, key: &str, value: &str) { if let Ok(x) = ValueRef::from_json(self, value) { - let addr = x.into_raw(self) as u64; - self.app_args.insert(key.to_string(), addr); + self.option_values.insert(key.to_string(), x); return; } - let addr = ValueRef::str(value).into_raw(self) as u64; - self.app_args.insert(key.to_string(), addr); + self.option_values + .insert(key.to_string(), ValueRef::str(value)); } pub fn builtin_option_reset(&mut self) { - for (_, x) in self.app_args.iter() { - if (*x) != 0 { - // kclvm_value_delete((*x) as *mut ValueRef); - } - } - self.app_args.clear(); + self.option_values.clear(); } } diff --git a/kclvm/runtime/src/stdlib/builtin_api.rs b/kclvm/runtime/src/stdlib/builtin_api.rs index 6ff756d08..7b017ec5b 100644 --- a/kclvm/runtime/src/stdlib/builtin_api.rs +++ b/kclvm/runtime/src/stdlib/builtin_api.rs @@ -41,7 +41,7 @@ pub unsafe extern "C" fn kclvm_builtin_option( args: *const kclvm_value_ref_t, kwargs: *const kclvm_value_ref_t, ) -> *mut kclvm_value_ref_t { - let ctx_ref = mut_ptr_as_ref(ctx); + let ctx = mut_ptr_as_ref(ctx); let args = ptr_as_ref(args); let kwargs = ptr_as_ref(kwargs); @@ -172,34 +172,24 @@ pub unsafe extern "C" fn kclvm_builtin_option( } if let Some(arg0) = get_call_arg_str(args, kwargs, 0, Some("key")) { - if let Some(x) = ctx_ref.app_args.get(&arg0) { - if *x == 0 { - return kclvm_value_Undefined(ctx); - } - - let opt_value = mut_ptr_as_ref((*x) as *mut kclvm_value_ref_t); - + if let Some(x) = ctx.option_values.get(&arg0) { if let Some(kwarg_type) = get_call_arg_str(args, kwargs, 1, Some("type")) { - return _value_to_type(opt_value, kwarg_type).into_raw(ctx_ref); + return _value_to_type(x, kwarg_type).into_raw(ctx); } - - return (*x) as *mut kclvm_value_ref_t; + return x.clone().into_raw(ctx); } else if let Some(kwarg_default) = get_call_arg(args, kwargs, 3, Some("default")) { if let Some(kwarg_type) = get_call_arg_str(args, kwargs, 1, Some("type")) { - return _value_to_type(&kwarg_default, kwarg_type).into_raw(ctx_ref); + return _value_to_type(&kwarg_default, kwarg_type).into_raw(ctx); } - - return kwarg_default.into_raw(ctx_ref); + return kwarg_default.into_raw(ctx); } } - let required = get_call_arg_bool(args, kwargs, 2, Some("required")).unwrap_or_default(); if required { let name = args.arg_i_str(0, Some("?".to_string())).unwrap(); panic!("option('{name}') must be initialized, try '-D {name}=?' argument"); } - - kclvm_value_None(ctx) + ValueRef::none().into_raw(ctx) } #[no_mangle] diff --git a/kclvm/runtime/src/value/iter.rs b/kclvm/runtime/src/value/iter.rs index 3ed64957f..8e56b7828 100644 --- a/kclvm/runtime/src/value/iter.rs +++ b/kclvm/runtime/src/value/iter.rs @@ -8,7 +8,6 @@ pub struct ValueIterator { pub len: usize, pub cur_key: ValueRef, pub cur_val: ValueRef, - pub end_val: *const ValueRef, pub keys: Vec, pub pos: i32, } @@ -19,7 +18,6 @@ impl Default for ValueIterator { len: 0, cur_key: Default::default(), cur_val: Default::default(), - end_val: std::ptr::null(), keys: Vec::new(), pos: 0, } @@ -35,33 +33,26 @@ impl ValueIterator { return Default::default(); } match *p.rc.borrow() { - Value::str_value(ref s) => { - ValueIterator { - len: s.len(), - cur_key: Default::default(), - cur_val: Default::default(), - end_val: 1 as *const ValueRef, // just as bool flag - keys: Vec::new(), - pos: 0, - } - } - Value::list_value(ref list) => { - ValueIterator { - len: list.values.len(), - cur_key: Default::default(), - cur_val: Default::default(), - end_val: 1 as *const ValueRef, // just as bool flag - keys: Vec::new(), - pos: 0, - } - } + Value::str_value(ref s) => ValueIterator { + len: s.len(), + cur_key: Default::default(), + cur_val: Default::default(), + keys: Vec::new(), + pos: 0, + }, + Value::list_value(ref list) => ValueIterator { + len: list.values.len(), + cur_key: Default::default(), + cur_val: Default::default(), + keys: Vec::new(), + pos: 0, + }, Value::dict_value(ref dict) => { let keys: Vec = dict.values.keys().map(|s| (*s).clone()).collect(); ValueIterator { len: dict.values.len(), cur_key: Default::default(), cur_val: Default::default(), - end_val: 1 as *const ValueRef, // just as bool flag keys, pos: 0, } @@ -73,7 +64,6 @@ impl ValueIterator { len: schema.config.values.len(), cur_key: Default::default(), cur_val: Default::default(), - end_val: 1 as *const ValueRef, // just as bool flag keys, pos: 0, } @@ -91,22 +81,14 @@ impl ValueIterator { if self.pos == 0 || self.pos > self.len as i32 { return Option::None; } - if !self.end_val.is_null() { - Some(&self.cur_key) - } else { - Option::None - } + Some(&self.cur_key) } pub fn value(&mut self) -> Option<&ValueRef> { if self.pos == 0 { return Option::None; } - if !self.end_val.is_null() { - Some(&self.cur_val) - } else { - Option::None - } + Some(&self.cur_val) } /// Get the next value, iterate key and value of the iterator. @@ -127,7 +109,6 @@ impl ValueIterator { return None; } if self.pos >= host.len() as i32 { - self.end_val = std::ptr::null(); return None; } match *host.rc.borrow() { @@ -135,14 +116,12 @@ impl ValueIterator { let ch = s.chars().nth(self.pos as usize).unwrap(); self.cur_key = ValueRef::int(self.pos as i64); self.cur_val = ValueRef::str(&ch.to_string()); - self.end_val = &self.cur_val; self.pos += 1; Some(&self.cur_val) } Value::list_value(ref list) => { self.cur_key = ValueRef::int(self.pos as i64); self.cur_val = list.values[self.pos as usize].clone(); - self.end_val = &self.cur_val; self.pos += 1; Some(&self.cur_val) } @@ -150,7 +129,6 @@ impl ValueIterator { let key = &self.keys[self.pos as usize]; self.cur_key = ValueRef::str(key); self.cur_val = dict.values[key].clone(); - self.end_val = &self.cur_val; self.pos += 1; Some(&self.cur_key) } @@ -158,7 +136,6 @@ impl ValueIterator { let key = &self.keys[self.pos as usize]; self.cur_key = ValueRef::str(key); self.cur_val = schema.config.values[key].clone(); - self.end_val = &self.cur_val; self.pos += 1; Some(&self.cur_key) }