Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: runtime option function memory leak and add gc function calling the evaluator #1355

Merged
merged 1 commit into from
May 24, 2024
Merged

fix: runtime option function memory leak and add gc function calling the evaluator #1355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion kclvm/api/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
3 changes: 3 additions & 0 deletions kclvm/runner/src/runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
3 changes: 1 addition & 2 deletions kclvm/runtime/src/api/kclvm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,15 +342,14 @@ pub struct Context {
/// Imported package path to check the cyclic import process.
pub imported_pkgpath: HashSet<String>,
/// Runtime arguments for the option function.
pub app_args: HashMap<String, u64>,
pub option_values: HashMap<String, ValueRef>,
/// All schema instances, the first key is the schema runtime type and
/// the second key is the schema instance package path
pub instances: IndexMap<String, IndexMap<String, Vec<ValueRef>>>,
/// All schema types
pub all_schemas: HashMap<String, SchemaType>,
/// Import graph
pub import_names: IndexMap<String, IndexMap<String, String>>,

/// A buffer to store plugin or hooks function calling results.
pub buffer: ContextBuffer,
/// Objects is to store all KCL object pointers at runtime.
Expand Down
11 changes: 10 additions & 1 deletion kclvm/runtime/src/context/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
}
}
}
14 changes: 4 additions & 10 deletions kclvm/runtime/src/stdlib/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Expand Down
24 changes: 7 additions & 17 deletions kclvm/runtime/src/stdlib/builtin_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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]
Expand Down
55 changes: 16 additions & 39 deletions kclvm/runtime/src/value/iter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,
pub pos: i32,
}
Expand All @@ -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,
}
Expand All @@ -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<String> = 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,
}
Expand All @@ -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,
}
Expand All @@ -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.
Expand All @@ -127,38 +109,33 @@ impl ValueIterator {
return None;
}
if self.pos >= host.len() as i32 {
self.end_val = std::ptr::null();
return None;
}
match *host.rc.borrow() {
Value::str_value(ref s) => {
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)
}
Value::dict_value(ref dict) => {
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)
}
Value::schema_value(ref schema) => {
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)
}
Expand Down
Loading