Skip to content

Commit e5cfc7a

Browse files
LukeMathWalkerLukeMathWalker
committed
Compile arr.h as a standalone static library
1 parent e4f18f2 commit e5cfc7a

10 files changed

Lines changed: 437 additions & 387 deletions

File tree

‎CMakeLists.txt‎

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ get_filename_component(binroot ${CMAKE_CURRENT_BINARY_DIR}/.. ABSOLUTE)
55

66
# Platform detection
77
if(APPLE)
8-
set(OS "macos")
8+
set(OS "macos")
99
elseif(UNIX)
10-
set(OS "linux")
10+
set(OS "linux")
1111
endif()
1212
message(STATUS "OS detected: ${OS}")
1313

@@ -38,28 +38,28 @@ endfunction()
3838

3939
# Define shared object setup function
4040
function(setup_shared_object_target target)
41-
if(APPLE)
42-
set_target_properties(${target} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup")
43-
# Force .so extension on macOS instead of .dylib
44-
set_target_properties(${target} PROPERTIES SUFFIX ".so")
45-
else()
46-
# We are building a shared library and want to verify that any reference to a symbol within the library will resolve to
47-
# the library's own definition, rather than to a definition in another shared library or the main executable.
48-
set_target_properties(${target} PROPERTIES LINK_FLAGS "-pthread -shared -Bsymbolic -Bsymbolic-functions")
49-
endif()
50-
set_target_properties(${target} PROPERTIES PREFIX "")
41+
if(APPLE)
42+
set_target_properties(${target} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup")
43+
# Force .so extension on macOS instead of .dylib
44+
set_target_properties(${target} PROPERTIES SUFFIX ".so")
45+
else()
46+
# We are building a shared library and want to verify that any reference to a symbol within the library will resolve to
47+
# the library's own definition, rather than to a definition in another shared library or the main executable.
48+
set_target_properties(${target} PROPERTIES LINK_FLAGS "-pthread -shared -Bsymbolic -Bsymbolic-functions")
49+
endif()
50+
set_target_properties(${target} PROPERTIES PREFIX "")
5151
endfunction()
5252

5353
# Define debug symbols extraction function
5454
function(extract_debug_symbols target)
55-
if(CMAKE_BUILD_TYPE STREQUAL "RelWithDebInfo" AND NOT APPLE)
56-
add_custom_command(TARGET ${target} POST_BUILD
55+
if(CMAKE_BUILD_TYPE STREQUAL "RelWithDebInfo" AND NOT APPLE)
56+
add_custom_command(TARGET ${target} POST_BUILD
5757
COMMAND cp $<TARGET_FILE:${target}> $<TARGET_FILE:${target}>.debug
5858
COMMAND objcopy --add-gnu-debuglink=$<TARGET_FILE:${target}>.debug $<TARGET_FILE:${target}>
5959
COMMAND strip -g $<TARGET_FILE:${target}>
6060
COMMENT "Extracting debug symbols from ${target}"
6161
)
62-
endif()
62+
endif()
6363
endfunction()
6464

6565
#----------------------------------------------------------------------------------------------
@@ -82,12 +82,12 @@ if(NOT DEFINED COORD_TYPE)
8282
endif()
8383

8484
if(COORD_TYPE STREQUAL "oss")
85-
set(BINDIR "${binroot}/search-community")
85+
set(BINDIR "${binroot}/search-community")
8686
elseif(COORD_TYPE STREQUAL "rlec")
87-
set(BINDIR "${binroot}/search-enterprise")
88-
add_compile_definitions(PRIVATE RS_CLUSTER_ENTERPRISE)
87+
set(BINDIR "${binroot}/search-enterprise")
88+
add_compile_definitions(PRIVATE RS_CLUSTER_ENTERPRISE)
8989
else()
90-
message(FATAL_ERROR "Invalid COORD_TYPE (='${COORD_TYPE}'). Should be either 'oss' or 'rlec'")
90+
message(FATAL_ERROR "Invalid COORD_TYPE (='${COORD_TYPE}'). Should be either 'oss' or 'rlec'")
9191
endif()
9292

9393
#----------------------------------------------------------------------------------------------
@@ -229,6 +229,7 @@ option(VECSIM_BUILD_TESTS "Build vecsim tests" OFF)
229229

230230
add_subdirectory(deps/VectorSimilarity)
231231
add_subdirectory(src/geometry)
232+
add_subdirectory(src/util/arr)
232233
add_subdirectory(src/util/hash)
233234
add_subdirectory(src/coord)
234235

@@ -265,6 +266,7 @@ add_library(rscore OBJECT ${SOURCES})
265266

266267
set(FINAL_OBJECTS
267268
$<TARGET_OBJECTS:trie>
269+
$<TARGET_OBJECTS:arr>
268270
$<TARGET_OBJECTS:rscore>
269271
$<TARGET_OBJECTS:rmutil>
270272
$<TARGET_OBJECTS:friso>
@@ -289,6 +291,7 @@ target_link_libraries(redisearch
289291
redisearch-hash
290292
VectorSimilarity
291293
redisearch-coord
294+
arr
292295
trie
293296
uv_a
294297
${BINDIR}/redisearch_rs/libredisearch_rs.a

‎src/redisearch_rs/trie_bencher/build.rs‎

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,19 +15,7 @@ fn main() {
1515

1616
// Construct the correct folder path based on OS and architecture
1717
let target_os = env::var("CARGO_CFG_TARGET_OS").unwrap();
18-
let lib_dir = {
19-
let target_arch = env::var("CARGO_CFG_TARGET_ARCH").unwrap();
20-
let target_arch = match target_arch.as_str() {
21-
"x86_64" => "x64",
22-
_ => &target_arch,
23-
};
2418

25-
root.join(format!(
26-
"bin/{target_os}-{target_arch}-release/search-community/deps/triemap"
27-
))
28-
};
29-
30-
assert!(std::fs::exists(lib_dir.join("libtrie.a")).unwrap());
3119
// There are several symbols exposed by `libtrie.a` that we don't
3220
// actually invoke (either directly or indirectly) in our benchmarks.
3321
// We provide a definition for the ones we need (e.g. Redis' allocation functions),
@@ -39,12 +27,32 @@ fn main() {
3927
if target_os != "macos" {
4028
println!("cargo:rustc-link-arg=-Wl,--unresolved-symbols=ignore-in-object-files");
4129
}
30+
31+
let bin_root = {
32+
let target_arch = env::var("CARGO_CFG_TARGET_ARCH").unwrap();
33+
let target_arch = match target_arch.as_str() {
34+
"x86_64" => "x64",
35+
_ => &target_arch,
36+
};
37+
38+
root.join(format!(
39+
"bin/{target_os}-{target_arch}-release/search-community/"
40+
))
41+
};
42+
43+
let libtrie_dir = bin_root.join("deps/triemap");
44+
let libtrie = libtrie_dir.join("libtrie.a");
45+
assert!(std::fs::exists(&libtrie).unwrap());
4246
println!("cargo:rustc-link-lib=static=trie");
43-
println!("cargo:rustc-link-search=native={}", lib_dir.display());
44-
println!(
45-
"cargo:rerun-if-changed={}",
46-
lib_dir.join("libtrie.a").display()
47-
);
47+
println!("cargo:rerun-if-changed={}", libtrie.display());
48+
println!("cargo:rustc-link-search=native={}", libtrie_dir.display());
49+
50+
let libarr_dir = bin_root.join("src/util/arr");
51+
let libarr = libarr_dir.join("libarr.a");
52+
assert!(std::fs::exists(&libarr).unwrap());
53+
println!("cargo:rustc-link-lib=static=arr");
54+
println!("cargo:rerun-if-changed={}", libarr.display());
55+
println!("cargo:rustc-link-search=native={}", libarr_dir.display());
4856

4957
let redis_modules = root.join("deps").join("RedisModulesSDK");
5058
let src = root.join("src");
@@ -57,6 +65,14 @@ fn main() {
5765
.to_str()
5866
.unwrap(),
5967
)
68+
.header(
69+
root.join("src")
70+
.join("util")
71+
.join("arr")
72+
.join("arr.h")
73+
.to_str()
74+
.unwrap(),
75+
)
6076
.clang_arg(format!("-I{}", src.display()))
6177
.clang_arg(format!("-I{}", deps.display()))
6278
.clang_arg(format!("-I{}", redis_modules.display()))

‎src/redisearch_rs/trie_bencher/src/bencher.rs‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -181,8 +181,7 @@ fn find_prefixes_rust_benchmark<M: Measurement>(
181181
) {
182182
let target = target.as_bytes();
183183
c.bench_function("Rust", |b| {
184-
// Set to avoid timing the drop phase to match the performance profile of the C bench.
185-
b.iter_with_large_drop(|| map.prefixes_iter(black_box(target)).collect::<Vec<_>>())
184+
b.iter(|| map.prefixes_iter(black_box(target)).collect::<Vec<_>>())
186185
});
187186
}
188187

@@ -194,10 +193,7 @@ fn find_prefixes_c_benchmark<M: Measurement>(
194193
let target = target.into_cstring();
195194
let view = target.as_view();
196195
let map = c_load_from_terms(terms);
197-
c.bench_function("C", |b| {
198-
// We are leaking memory here, since we don't free the pointer to the result array.
199-
b.iter_with_large_drop(|| map.find_prefixes(view).unwrap())
200-
});
196+
c.bench_function("C", |b| b.iter(|| map.find_prefixes(view).unwrap()));
201197
}
202198

203199
fn find_rust_benchmark<M: Measurement>(

‎src/redisearch_rs/trie_bencher/src/c_map.rs‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
//! and providing access to the raw pointer and length of the string as needed by the C API.
1414
use std::ffi::{CStr, CString, c_char, c_void};
1515

16+
use crate::ffi::{array_free, array_new_sz};
17+
1618
#[repr(transparent)]
1719
/// A thin wrapper around the C TrieMap implementation to ensure that the map is properly initialized and cleaned up.
1820
pub struct CTrieMap(*mut crate::ffi::TrieMap);
@@ -44,12 +46,21 @@ impl CTrieMap {
4446
unsafe { crate::ffi::TrieMap_Delete(self.0, term.ptr(), term.len(), Some(do_not_free)) }
4547
}
4648

47-
pub fn find_prefixes(&self, term: TrieTermView) -> Result<*mut *mut c_void, ()> {
48-
let mut results: *mut *mut c_void = std::ptr::null_mut();
49+
pub fn find_prefixes(&self, term: TrieTermView) -> Result<PrefixesValues, ()> {
50+
let mut results: *mut *mut c_void = {
51+
// Here we are emulating the behaviour of the `array_new` macro, which we can't
52+
// invoke directly on the Rust side.
53+
let raw = unsafe { array_new_sz(std::mem::size_of::<*mut c_void>() as u32, 1, 0) };
54+
raw.cast()
55+
};
4956
let error_code = unsafe {
5057
crate::ffi::TrieMap_FindPrefixes(self.0, term.ptr(), term.len(), &mut results)
5158
};
52-
if error_code > 0 { Err(()) } else { Ok(results) }
59+
if error_code > 0 {
60+
Err(())
61+
} else {
62+
Ok(PrefixesValues(results))
63+
}
5364
}
5465

5566
pub fn n_nodes(&self) -> usize {
@@ -71,6 +82,15 @@ impl Drop for CTrieMap {
7182
}
7283
}
7384

85+
/// The values attached to the prefixes retrieved by [`CTrieMap::find_prefixes`].
86+
pub struct PrefixesValues(*mut *mut c_void);
87+
88+
impl Drop for PrefixesValues {
89+
fn drop(&mut self) {
90+
unsafe { array_free(*self.0) };
91+
}
92+
}
93+
7494
unsafe extern "C" fn do_nothing(oldval: *mut c_void, _newval: *mut c_void) -> *mut c_void {
7595
// Just return the old value, since it's a null pointer and we don't want
7696
// the C map implementation to try to free it.

‎src/redisearch_rs/trie_bencher/src/ffi.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ mod bindings {
2929
// It allows us to keep the bindings module private while still exposing the necessary functions and types.
3030
pub(crate) use bindings::{
3131
NewTrieMap, TrieMap, TrieMap_Add, TrieMap_Delete, TrieMap_ExactMemUsage, TrieMap_Find,
32-
TrieMap_FindPrefixes, TrieMap_Free,
32+
TrieMap_FindPrefixes, TrieMap_Free, array_free, array_new_sz,
3333
};
3434
// used in outside binary crate (main.rs)
3535
pub use bindings::TRIEMAP_NOTFOUND;

‎src/util/arr.c‎

Lines changed: 0 additions & 15 deletions
This file was deleted.

0 commit comments

Comments
 (0)