Merge 'Fix memory leaks, make extension types more efficient' from Pr…

…eston Thorpe

I was baffled previously, because any time that `free` was called on a
type from an extension, it would hang even when I knew it wasn't in use
any longer, and hadn't been double free'd.
After #737 was merged, I tried it again and noticed that it would no
longer hang... but only for extensions that were staticly linked.
Then I realized that we are using a global allocator, that likely wasn't
getting used in the shared library that is built separately that won't
inherit from our global allocator in core, causing some symbol mismatch
and the subsequent hanging on calls to `free`.
This PR adds the global allocator to extensions behind a feature flag in
the macro that will prevent it from being used in `wasm` and staticly
linked environments where it would conflict with limbos normal global
allocator. This allows us to properly free the memory from returning
extension functions over FFI.
This PR also changes the Extension type to a union field so we can store
int + float values inline without boxing them.
any additional tips or thoughts anyone else has on improving this would
be appreciated 👍

Closes #803

core/types.rs

@@ -130,38 +130,41 @@ impl OwnedValue {
         }
     }
 
-    pub fn from_ffi(v: &ExtValue) -> Self {
+    pub fn from_ffi(v: &ExtValue) -> Result<Self> {
         match v.value_type() {
-            ExtValueType::Null => OwnedValue::Null,
+            ExtValueType::Null => Ok(OwnedValue::Null),
             ExtValueType::Integer => {
                 let Some(int) = v.to_integer() else {
-                    return OwnedValue::Null;
+                    return Ok(OwnedValue::Null);
                 };
-                OwnedValue::Integer(int)
+                Ok(OwnedValue::Integer(int))
             }
             ExtValueType::Float => {
                 let Some(float) = v.to_float() else {
-                    return OwnedValue::Null;
+                    return Ok(OwnedValue::Null);
                 };
-                OwnedValue::Float(float)
+                Ok(OwnedValue::Float(float))
             }
             ExtValueType::Text => {
                 let Some(text) = v.to_text() else {
-                    return OwnedValue::Null;
+                    return Ok(OwnedValue::Null);
                 };
-                OwnedValue::build_text(Rc::new(text))
+                Ok(OwnedValue::build_text(Rc::new(text.to_string())))
             }
             ExtValueType::Blob => {
                 let Some(blob) = v.to_blob() else {
-                    return OwnedValue::Null;
+                    return Ok(OwnedValue::Null);
                 };
-                OwnedValue::Blob(Rc::new(blob))
+                Ok(OwnedValue::Blob(Rc::new(blob)))
             }
             ExtValueType::Error => {
-                let Some(err) = v.to_error() else {
-                    return OwnedValue::Null;
+                let Some(err) = v.to_error_details() else {
+                    return Ok(OwnedValue::Null);
                 };
-                OwnedValue::Text(LimboText::new(Rc::new(err)))
+                match err {
+                    (_, Some(msg)) => Err(LimboError::ExtensionError(msg)),
+                    (code, None) => Err(LimboError::ExtensionError(code.to_string())),
+                }
             }
         }
     }
@@ -181,13 +184,15 @@ pub enum AggContext {
 const NULL: OwnedValue = OwnedValue::Null;
 
 impl AggContext {
-    pub fn compute_external(&mut self) {
+    pub fn compute_external(&mut self) -> Result<()> {
         if let Self::External(ext_state) = self {
             if ext_state.finalized_value.is_none() {
                 let final_value = unsafe { (ext_state.finalize_fn)(ext_state.state) };
-                ext_state.cache_final_value(OwnedValue::from_ffi(&final_value));
+                ext_state.cache_final_value(OwnedValue::from_ffi(&final_value)?);
+                unsafe { final_value.free() };
             }
         }
+        Ok(())
     }
 
     pub fn final_value(&self) -> &OwnedValue {

core/vdbe/mod.rs

@@ -166,8 +166,16 @@ macro_rules! call_external_function {
     ) => {{
         if $arg_count == 0 {
             let result_c_value: ExtValue = unsafe { ($func_ptr)(0, std::ptr::null()) };
-            let result_ov = OwnedValue::from_ffi(&result_c_value);
-            $state.registers[$dest_register] = result_ov;
+            match OwnedValue::from_ffi(&result_c_value) {
+                Ok(result_ov) => {
+                    $state.registers[$dest_register] = result_ov;
+                    unsafe { result_c_value.free() };
+                }
+                Err(e) => {
+                    unsafe { result_c_value.free() };
+                    return Err(e);
+                }
+            }
         } else {
             let register_slice = &$state.registers[$start_reg..$start_reg + $arg_count];
             let mut ext_values: Vec<ExtValue> = Vec::with_capacity($arg_count);
@@ -177,8 +185,16 @@ macro_rules! call_external_function {
             }
             let argv_ptr = ext_values.as_ptr();
             let result_c_value: ExtValue = unsafe { ($func_ptr)($arg_count as i32, argv_ptr) };
-            let result_ov = OwnedValue::from_ffi(&result_c_value);
-            $state.registers[$dest_register] = result_ov;
+            match OwnedValue::from_ffi(&result_c_value) {
+                Ok(result_ov) => {
+                    $state.registers[$dest_register] = result_ov;
+                    unsafe { result_c_value.free() };
+                }
+                Err(e) => {
+                    unsafe { result_c_value.free() };
+                    return Err(e);
+                }
+            }
         }
     }};
 }
@@ -1556,7 +1572,7 @@ impl Program {
                             AggFunc::Min => {}
                             AggFunc::GroupConcat | AggFunc::StringAgg => {}
                             AggFunc::External(_) => {
-                                agg.compute_external();
+                                agg.compute_external()?;
                             }
                         },
                         OwnedValue::Null => {

extensions/core/README.md

@@ -19,13 +19,18 @@ Add the crate to your `Cargo.toml`:
 ```toml
 [dependencies]
 limbo_ext = { path = "path/to/limbo/extensions/core" } # temporary until crate is published
+
+# mimalloc is required if you intend on linking dynamically. It is imported for you by the register_extension
+# macro, so no configuration is needed. But it must be added to your Cargo.toml
+[target.'cfg(not(target_family = "wasm"))'.dependencies]
+mimalloc = { version = "*", default-features = false }
 ```
 
-**NOTE** Crate must be of type `cdylib`
+**NOTE** Crate must be of type `cdylib` if you wish to link dynamically
 
 ```
 [lib]
-crate-type = ["cdylib"]
+crate-type = ["cdylib", "lib"]
 ```
 
 `cargo build` will output a shared library that can be loaded with `.load target/debug/libyour_crate_name`