Implement defrag support for `IValue`. #1

MeirShpilraien · 2024-09-03T08:05:58Z

The PR adds support for defrag on IValue. The defrag uses the provided DefragAllocator to reallocate all the memory of the IValue.

Shared String Support

In general, it is considered easy to reallocate all the pointer of the IValue the only exception is the shared strings which we can not simply reallocate and free the old pointer because there might be more references that points to it.

To solve this issue we provide a way to clean the shared strings dictionary. This will cause new strings to be allocated from scratch and old shared strings will be freed when there will have no more references.

Notice that this means that now a shared string might not actually be shared between all jsons. Two json might have the same strings but with a different instance. This means that we can no longer count on pointer value for strings hash function, we must perform the hash function on the string value itself.

The PR adds support for defrag on `IValue`. The defrag uses the provided `DefragAllocator` to reallocate all the memory of the `IValue`. ### Shared String Support In general, it is considered easy to reallocate all the pointer of the `IValue` the only exception is the shared strings which we can not simply reallocate and free the old pointer because there might be more references that points to it. To solve this issue we provide a way to clean the shared strings dictionary. This will cause new strings to be allocated from scratch and old shared strings will be freed when there will have no more references. Notice that this means that now a shared string might not actually be shared between all jsons. Two json might have the same strings but with a different instance. This means that we can no longer count on pointer value for strings hash function, we must perform the hash function on the string value itself.

The PR adds support for active defrag on RedisJSON. A pre condition for this PR is that the following PR's will be megred: * [Redis defrad module API extentions](redis/redis#13509) * [redismodule-rs support for active defrag API](RedisLabsModules/redismodule-rs#387) * [IJSON support for defrag](RedisJSON/ijson#1) The PR register defrag function on the json datatype and uses the new capability of ISON to defrag the key. **Notice**: * that increamental defrag of the json key is **not** support. In order to support it we need to implement the free_effort callback. This is not trivial and even if it was it would have cause the json object to potentially be freed when the GIL is not hold, which violate the assumption of our shared string implementation (it is not thread safe). We leave it for future improvment. * If we run on a Redis version that do not support the defrag start callback, we can still partially support defrag. In that case the IJSON object will be defraged but the shared strings dictionaty will not be reinitialize. This basically means that shared strings will not be defraged. Tests were added to cover the new functionality.

ephraimfeldblum · 2024-09-03T12:19:00Z

src/number.rs

+impl<A: DefragAllocator> Defrag<A> for INumber {
+    fn defrag(mut self, defrag_allocator: &mut A) -> Self {
+        let hd = self.header();
+        if hd.type_ == NumberType::Static {
+            return self;
+        }
+        unsafe {
+            let ptr = self.0.ptr().cast::<Header>();
+            self.0.set_ptr(
+                defrag_allocator.realloc_ptr(ptr.cast(), Self::layout((*ptr).type_).unwrap()),
+            )
+        };
+        self
+    }
+}


Three comments:
Use expect instead of unwrap.
Would it be possible to get the ptr from the Header itself instead of going through the IValue::ptr?
And would it be possible to reuse the already extracted type instead of accessing it again through ptr?

eg something like:

Suggested change

impl<A: DefragAllocator> Defrag<A> for INumber {

fn defrag(mut self, defrag_allocator: &mut A) -> Self {

let hd = self.header();

if hd.type_ == NumberType::Static {

return self;

}

unsafe {

let ptr = self.0.ptr().cast::<Header>();

self.0.set_ptr(

defrag_allocator.realloc_ptr(ptr.cast(), Self::layout((*ptr).type_).unwrap()),

)

};

self

}

}

impl<A: DefragAllocator> Defrag<A> for INumber {

fn defrag(mut self, defrag_allocator: &mut A) -> Self {

let hd = self.header();

match hd.type_ {

NumberType::Static => self,

t => {

let ptr = hd.ptr();

let layout = Self::layout(t).expect(...);

unsafe {

self.0.set_ptr(defrag_allocator.realloc_ptr(ptr.cast(), layout));

}

self

}

}

}

}

ephraimfeldblum · 2024-09-03T12:27:26Z

src/object.rs

+            return self;
+        }
+        let header = unsafe { self.header_mut() };
+        let slit_header = header.split_mut();


split_header

ephraimfeldblum · 2024-09-03T12:45:34Z

src/unsafe_string.rs

@@ -160,10 +168,10 @@ impl IString {
            .pad_to_align())
    }

-    fn alloc(s: &str) -> *mut Header {
+    fn alloc<A: FnMut(Layout) -> *mut u8>(s: &str, mut allocator: A) -> *mut Header {


Should the Allocator not be FnOnce?

ephraimfeldblum · 2024-09-03T12:46:24Z

src/unsafe_string.rs

@@ -175,29 +183,31 @@ impl IString {
        }
    }

-    fn dealloc(ptr: *mut Header) {
+    fn dealloc<D: FnMut(*mut u8, Layout)>(ptr: *mut Header, mut deallocator: D) {


ephraimfeldblum · 2024-09-03T12:46:32Z

src/unsafe_string.rs

-    /// Converts a `&str` to an `IString` by interning it in the global string cache.
-    #[must_use]
-    pub fn intern(s: &str) -> Self {
+    fn intern_with_allocator<A: FnMut(Layout) -> *mut u8>(s: &str, allocator: A) -> Self {


ephraimfeldblum · 2024-09-03T12:47:02Z

src/unsafe_string.rs

@@ -242,26 +252,36 @@ impl IString {
            unsafe { self.0.raw_copy() }
        }
    }
-    pub(crate) fn drop_impl(&mut self) {
+
+    fn drop_impl_with_deallocator<D: FnMut(*mut u8, Layout)>(&mut self, deallocator: D) {


ephraimfeldblum · 2024-09-03T12:51:57Z

src/unsafe_string.rs

        assert!((s.len() as u64) < (1 << 48));
        unsafe {
-            let ptr = alloc(Self::layout(s.len()).unwrap()).cast::<Header>();
+            let ptr = allocator(Self::layout(s.len()).unwrap()).cast::<Header>();


ephraimfeldblum · 2024-09-03T12:52:36Z

src/array.rs

+        unsafe {
+            let new_ptr = defrag_allocator.realloc_ptr(
+                self.0.ptr(),
+                Self::layout((*self.0.ptr().cast::<Header>()).cap).unwrap(),


ephraimfeldblum · 2024-09-03T12:54:55Z

src/value.rs

@@ -208,6 +210,35 @@ pub enum ValueType {
 unsafe impl Send for IValue {}
 unsafe impl Sync for IValue {}

+impl<A: DefragAllocator> Defrag<A> for IValue {
+    fn defrag(self, defrag_allocator: &mut A) -> Self {
+        match self.type_() {


match self.destructure()

ephraimfeldblum · 2024-09-03T13:16:17Z

src/value.rs

+    // Safety: Must be an array
+    unsafe fn to_array_unchecked(self) -> IArray {
+        IArray(self)
+    }
+
+    // Safety: Must be an array
+    unsafe fn to_object_unchecked(self) -> IObject {
+        IObject(self)
+    }
+
+    unsafe fn to_string_unchecked(self) -> IString {
+        IString(self)
+    }
+
+    unsafe fn to_number_unchecked(self) -> INumber {
+        INumber(self)
+    }


Are these necessary anymore?

The PR adds support for active defrag on RedisJSON. A pre condition for this PR is that the following PR's will be megred: * [Redis defrad module API extentions](redis/redis#13509) * [redismodule-rs support for active defrag API](RedisLabsModules/redismodule-rs#387) * [IJSON support for defrag](RedisJSON/ijson#1) The PR register defrag function on the json datatype and uses the new capability of ISON to defrag the key. **Notice**: * Increamental defrag of the json key is **not** support. In order to support it we need to implement the free_effort callback. This is not trivial and even if it was it would have cause the json object to potentially be freed when the GIL is not hold, which violate the assumption of our shared string implementation (it is not thread safe). We leave it for future improvment. * If we run on a Redis version that do not support the defrag start callback, we can still partially support defrag. In that case the IJSON object will be defraged but the shared strings dictionatrywill not be reinitialize. This basically means that shared strings will not be defraged. Tests were added to cover the new functionality.

The PR adds support for active defrag on RedisJSON. A pre condition for this PR is that the following PR's will be megred: * [Redis defrad module API extentions](redis/redis#13509) * [redismodule-rs support for active defrag API](RedisLabsModules/redismodule-rs#387) * [IJSON support for defrag](RedisJSON/ijson#1) The PR register defrag function on the json datatype and uses the new capability of ISON to defrag the key. **Notice**: * Increamental defrag of the json key is **not** support. In order to support it we need to implement the free_effort callback. This is not trivial and even if it was it would have cause the json object to potentially be freed when the GIL is not hold, which violate the assumption of our shared string implementation (it is not thread safe). We leave it for future improvment. * If we run on a Redis version that do not support the defrag start callback, we can still partially support defrag. In that case the IJSON object will be defraged but the shared strings dictionatrywill not be reinitialize. This basically means that shared strings will not be defraged. Tests were added to cover the new functionality. (cherry picked from commit 6539e1e)

MeirShpilraien requested a review from ephraimfeldblum September 3, 2024 08:05

MeirShpilraien mentioned this pull request Sep 3, 2024

RED-134847 support for active defrag for RedisJSON. RedisJSON/RedisJSON#1262

Merged

1 task

Format

cb1c7dc

ephraimfeldblum requested changes Sep 3, 2024

View reviewed changes

Review fixes

22e60c1

MeirShpilraien requested a review from ephraimfeldblum September 3, 2024 13:12

ephraimfeldblum reviewed Sep 3, 2024

View reviewed changes

Review fixes

9179ff4

MeirShpilraien requested a review from ephraimfeldblum September 3, 2024 14:23

ephraimfeldblum approved these changes Sep 3, 2024

View reviewed changes

MeirShpilraien merged commit eede48f into unsafe_string Sep 3, 2024

MeirShpilraien mentioned this pull request Sep 16, 2024

RED-134847 support for active defrag for RedisJSON. (#1262) RedisJSON/RedisJSON#1268

Closed

MeirShpilraien mentioned this pull request Sep 16, 2024

RED-134847 support for active defrag for RedisJSON. (#1262) RedisJSON/RedisJSON#1269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement defrag support for `IValue`. #1

Implement defrag support for `IValue`. #1

MeirShpilraien commented Sep 3, 2024 •

edited

Loading

ephraimfeldblum Sep 3, 2024

ephraimfeldblum Sep 3, 2024

ephraimfeldblum Sep 3, 2024

ephraimfeldblum Sep 3, 2024

ephraimfeldblum Sep 3, 2024

ephraimfeldblum Sep 3, 2024

ephraimfeldblum Sep 3, 2024

ephraimfeldblum Sep 3, 2024

ephraimfeldblum Sep 3, 2024

ephraimfeldblum Sep 3, 2024

Implement defrag support for IValue. #1

Implement defrag support for IValue. #1

Conversation

MeirShpilraien commented Sep 3, 2024 • edited Loading

Shared String Support

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement defrag support for `IValue`. #1

Implement defrag support for `IValue`. #1

MeirShpilraien commented Sep 3, 2024 •

edited

Loading