GO: performance improvement (valkey-io#2196)

* Performance improvement for GO client

- Use the go string internal pointer in Rust code
- Do not copy go string pointers when building Redis command in lib.rs
- Disabled go1.18

Signed-off-by: Eran Ifrah <[email protected]>

.github/workflows/go.yml

@@ -52,7 +52,6 @@ jobs:
             fail-fast: false
             matrix:
                 go:
-                    # - '1.18.10'
                     - '1.22.0'
                 engine: ${{ fromJson(needs.load-engine-matrix.outputs.matrix) }}
                 host:
@@ -130,7 +129,6 @@ jobs:
             fail-fast: false
             matrix:
                 go:
-                    - 1.18.10
                     - 1.22.0
         runs-on: ubuntu-latest
         container: amazonlinux:latest

go/Makefile

@@ -28,11 +28,26 @@ install-tools: install-tools-go1.22.0
 
 build: build-glide-client generate-protobuf
 	go build ./...
+	cd benchmarks && go build -ldflags="-w" ./...
+
+build-debug: build-glide-client-debug generate-protobuf
+	go build -gcflags "-l -N" ./...
+	cd benchmarks && go build -gcflags "-l -N" ./...
+
+clean:
+	go clean
+	rm -f lib.h
+	rm -f benchmarks/benchmarks
+
 
 build-glide-client:
 	cargo build --release
 	cbindgen --config cbindgen.toml --crate glide-rs --output lib.h
 
+build-glide-client-debug:
+	cargo build
+	cbindgen --config cbindgen.toml --crate glide-rs --output lib.h
+
 generate-protobuf:
 	mkdir -p protobuf
 	protoc --proto_path=../glide-core/src/protobuf \
@@ -58,7 +73,7 @@ format:
 	golines -w --shorten-comments -m 127 .
 
 test:
-	LD_LIBRARY_PATH=$(shell find . -name libglide_rs.so|tail -1|xargs dirname|xargs readlink -f):${LD_LIBRARY_PATH} \
+	LD_LIBRARY_PATH=$(shell find . -name libglide_rs.so|grep -w release|tail -1|xargs dirname|xargs readlink -f):${LD_LIBRARY_PATH} \
 	go test -v -race ./...
 
 # Note: this task is no longer run by CI because:

go/api/base_client.go

@@ -100,7 +100,6 @@ func (client *baseClient) executeCommand(requestType C.RequestType, args []strin
 	}
 
 	cArgs, argLengths := toCStrings(args)
-	defer freeCStrings(cArgs)
 
 	resultChannel := make(chan payload)
 	resultChannelPtr := uintptr(unsafe.Pointer(&resultChannel))
@@ -120,23 +119,26 @@ func (client *baseClient) executeCommand(requestType C.RequestType, args []strin
 	return payload.value, nil
 }
 
-// TODO: Handle passing the arguments as strings without assuming null termination assumption.
-func toCStrings(args []string) ([]*C.char, []C.ulong) {
-	cStrings := make([]*C.char, len(args))
+// Convert `s` of type `string` into `[]byte`
+func StringToBytes(s string) []byte {
+	p := unsafe.StringData(s)
+	b := unsafe.Slice(p, len(s))
+	return b
+}
+
+// Zero copying conversion from go's []string into C pointers
+func toCStrings(args []string) ([]C.uintptr_t, []C.ulong) {
+	cStrings := make([]C.uintptr_t, len(args))
 	stringLengths := make([]C.ulong, len(args))
 	for i, str := range args {
-		cStrings[i] = C.CString(str)
+		bytes := StringToBytes(str)
+		ptr := uintptr(unsafe.Pointer(&bytes[0]))
+		cStrings[i] = C.uintptr_t(ptr)
 		stringLengths[i] = C.size_t(len(str))
 	}
 	return cStrings, stringLengths
 }
 
-func freeCStrings(cArgs []*C.char) {
-	for _, arg := range cArgs {
-		C.free(unsafe.Pointer(arg))
-	}
-}
-
 func (client *baseClient) Set(key string, value string) (string, error) {
 	result, err := client.executeCommand(C.Set, []string{key, value})
 	if err != nil {

go/go.mod

@@ -1,6 +1,6 @@
 module github.com/valkey-io/valkey-glide/go/glide
 
-go 1.18
+go 1.20
 
 require (
 	github.com/stretchr/testify v1.8.4

go/src/lib.rs

@@ -250,25 +250,32 @@ pub unsafe extern "C" fn free_command_response(command_response_ptr: *mut Comman
 ///
 /// # Safety
 ///
-/// `free_error_message` can only be called once per `error_message`. Calling it twice is undefined behavior, since the address will be freed twice.
+/// `free_error_message` can only be called once per `error_message`. Calling it twice is undefined
+/// behavior, since the address will be freed twice.
 #[no_mangle]
 pub unsafe extern "C" fn free_error_message(error_message: *mut c_char) {
     assert!(!error_message.is_null());
     drop(unsafe { CString::from_raw(error_message as *mut c_char) });
 }
 
 /// Converts a double pointer to a vec.
-unsafe fn convert_double_pointer_to_vec(
-    data: *const *const c_char,
+///
+/// # Safety
+///
+/// `convert_double_pointer_to_vec` returns a `Vec` of u8 slice which holds pointers of `go`
+/// strings. The returned `Vec<&'a [u8]>` is meant to be copied into Rust code. Storing them
+/// for later use will cause the program to crash as the pointers will be freed by go's gc
+unsafe fn convert_double_pointer_to_vec<'a>(
+    data: *const *const c_void,
     len: c_ulong,
     data_len: *const c_ulong,
-) -> Vec<Vec<u8>> {
+) -> Vec<&'a [u8]> {
     let string_ptrs = unsafe { from_raw_parts(data, len as usize) };
     let string_lengths = unsafe { from_raw_parts(data_len, len as usize) };
-    let mut result: Vec<Vec<u8>> = Vec::new();
+    let mut result = Vec::<&[u8]>::with_capacity(string_ptrs.len());
     for (i, &str_ptr) in string_ptrs.iter().enumerate() {
         let slice = unsafe { from_raw_parts(str_ptr as *const u8, string_lengths[i] as usize) };
-        result.push(slice.to_vec());
+        result.push(slice);
     }
     result
 }
@@ -293,25 +300,30 @@ pub unsafe extern "C" fn command(
     channel: usize,
     command_type: RequestType,
     arg_count: c_ulong,
-    args: *const *const c_char,
+    args: *const usize,
     args_len: *const c_ulong,
 ) {
     let client_adapter =
         unsafe { Box::leak(Box::from_raw(client_adapter_ptr as *mut ClientAdapter)) };
-    // The safety of this needs to be ensured by the calling code. Cannot dispose of the pointer before all operations have completed.
+    // The safety of this needs to be ensured by the calling code. Cannot dispose of the pointer before
+    // all operations have completed.
     let ptr_address = client_adapter_ptr as usize;
 
-    let arg_vec = unsafe { convert_double_pointer_to_vec(args, arg_count, args_len) };
+    let arg_vec =
+        unsafe { convert_double_pointer_to_vec(args as *const *const c_void, arg_count, args_len) };
 
     let mut client_clone = client_adapter.client.clone();
-    client_adapter.runtime.spawn(async move {
-        let mut cmd = command_type
-            .get_command()
-            .expect("Couldn't fetch command type");
-        for slice in arg_vec {
-            cmd.arg(slice);
-        }
 
+    // Create the command outside of the task to ensure that the command arguments passed
+    // from "go" are still valid
+    let mut cmd = command_type
+        .get_command()
+        .expect("Couldn't fetch command type");
+    for command_arg in arg_vec {
+        cmd.arg(command_arg);
+    }
+
+    client_adapter.runtime.spawn(async move {
         let result = client_clone.send_command(&cmd, None).await;
         let client_adapter = unsafe { Box::leak(Box::from_raw(ptr_address as *mut ClientAdapter)) };
         let value = match result {