Add free() for raft node #423

jayzhan211 · 2021-03-18T08:50:32Z

Close #419 and #448 .

Add two API

free_resources(id: u64)
free_resources_for_all()

Signed-off-by: jayzhan jayzhan211@gmail.com

jayzhan211 · 2021-03-18T08:50:45Z

how to add test? or not

jayzhan211 · 2021-03-18T08:53:22Z

src/tracker/inflights.rs

+    /// free buffer memory
+    #[inline]
+    pub fn shrink(&mut self) {
+        self.buffer.shrink_to_fit();


After shrink the capacity will change, the size for determine full() will be effected, is it fine?

It's wrong. It should be able to reallocate when new messages need to be sent.

BusyJay · 2021-03-18T08:59:35Z

src/raft.rs

@@ -2678,4 +2678,27 @@ impl<T: Storage> Raft<T> {
    pub fn uncommitted_size(&self) -> usize {
        self.uncommitted_state.uncommitted_size
    }
+
+    /// Free resources for all the raft node
+    pub fn free_resources_for_all(&mut self) {


Just this method is enough. Choosing target id is too verbose.

BusyJay · 2021-03-18T09:00:55Z

Tests should be added to check if flow control still work as expected before and after calling shrink.

BusyJay · 2021-04-02T12:54:05Z

src/tracker/inflights.rs

+    pub fn free_mem(&mut self) {
+        let mut v = Vec::with_capacity(self.count);
+        for i in 0..self.count {
+            v.push(self.buffer[self.start + i])


I don't get it. v doesn't seem to be used, and why append the element from buffer to v? On the other hand, self.buffer should be dropped to free memory.

I am trying to re-create an buffer with length & capacity equal to count, in order to decrease the allocated memory,
and remain only from start to start + count - 1

On the other hand, buffer should be dropped to free memory

If count is zero, buffer will be Vec with capacity 0, I am not sure what is the implementation inside, but I guess there is no memory allocated?
If count is positive, we should at least remain count sized Vec (?

shrink_to_fit that mentioned before, I think it will do nothing if the Vec is full once

What is the issue left related to the memory problem? Is #10156?

What is 10156?

Sorry, is this tikv/tikv#10156, found it under tikv/rfcs#59

The left issue is unchanged for raft-rs. Memory usage of Inflight is too large if there are lots of raft peers.

@BusyJay do you mean free_mem should only free empty inflights, but keep no-empty inflights unchanged?

I have discussed with Jay. He thinks the current implemenatation is OK. BTW how about using vec.extend_from_slice instead?

hicqu · 2021-06-28T12:12:15Z

How about lazily allocate memory instead of adding free_* functions? @jayzhan211

hicqu · 2021-06-29T08:12:36Z

How about this solution:
for an given Inflight,

allocate memory only if necessary
reallocate memory if more items are pushed
free and malloc again after free_to is called, and shrink the buffer if necessary

Or this:

Allocate link-list nodes on heap, every node carries 16 u64 numbers

Any suggestions @BusyJay

BusyJay · 2021-07-05T07:06:08Z

src/tracker/inflights.rs

+
+    /// reallocate buffer memory
+    #[inline]
+    pub fn reallocate(&mut self) {


Progress should reallocate when necessary.

Currently, buffer will be reallocated when you call this function, what is other situation when we need reallocation?

hicqu · 2021-07-05T09:01:50Z

@jayzhan211 we can try this solution:

free_mem shouldn't re-allocate memory. So for an un-empty Inflights, we can
1. leave it unchanged, or
2. Call shrink_to_fit for it
Both of them are OK.
Inflights::reallocate is not necessary. Memory can be allocated when Inflights::add is called.
Call free_mem when a peer become follower.

@BusyJay please help to confirm whether it fits with your design or not.

hicqu · 2021-07-09T08:02:18Z

@jayzhan211 any updates?

jayzhan211 · 2021-07-09T08:23:03Z

@jayzhan211 any updates?

Does it fit @BusyJay 's design

BusyJay · 2021-07-09T08:33:58Z

@jayzhan211 we can try this solution:

free_mem shouldn't re-allocate memory. So for an un-empty Inflights, we can

leave it unchanged, or

Call shrink_to_fit for it

Both of them are OK.

Inflights::reallocate is not necessary. Memory can be allocated when Inflights::add is called.

Call free_mem when a peer become follower.

@BusyJay please help to confirm whether it fits with your design or not.

LGTM, thanks!

jayzhan211 · 2021-07-09T13:09:22Z

I have difficult for adding test (not famailiar with APIs).
Maybe add it afterall.

jayzhan211 · 2021-07-09T13:11:08Z

src/tracker/inflights.rs

+        let len = self.buffer.len();
+        for _ in self.start + self.count..len {
+            self.buffer.pop();
+        }


We initialize the buffer with fixed capacity, so we need to remove element for shrink to fit to reduce allocated memory, any better ways?

How about don't touch it if it's not empty? So the resource can be freed later, and no allocations are introduced.

How about don't touch it if it's not empty?

I am not sure how frequently the buffer is empty. If it happens a lot, this approach is good enough.

If most of the time the buffer is non-empty but small, then memory might be wasted (0..start - 1).

jayzhan211 · 2021-07-09T13:15:15Z

harness/tests/integration_cases/test_raft_flow_control.rs

+//             .expect("");
+//         r.read_messages();
+//     }
+//


After this line, inflights 2 (follower) is filled with 10 element, inflights 1 (leader) is empty, so it seems that we need to transfer leader from 2 to 1, then transfer 1 to 2 again, so peer 2 will call free_mem ?

Signed-off-by: qupeng <qupeng@pingcap.com>

hicqu · 2021-07-19T06:41:28Z

@jayzhan211 I have send a patch PR to your code. Could you take a look?

Signed-off-by: qupeng <qupeng@pingcap.com>

BusyJay · 2021-07-20T08:02:58Z

harness/tests/integration_cases/test_raft_flow_control.rs

+    r.step(resp).unwrap();
+
+    // Test `maybe_free_inflight_buffers` works as expected.
+    r.maybe_free_inflight_buffers();


It seems the case can pass even the function does nothing.

BusyJay · 2021-07-20T08:06:02Z

src/tracker/inflights.rs

@@ -35,6 +38,7 @@ impl Clone for Inflights {
            start: self.start,
            count: self.count,
            buffer,
+            cap: self.cap,


L36 can be also removed.

The buffer must have it's capacity set correctly on clone, normally it does not.

If it is not true, I think we can use derive(Clone) for inflight

Now that capacity is maintained explicitly, so the statement is not valid anymore. derive(Clone) is a good idea!.

hicqu · 2021-07-20T12:26:15Z

harness/tests/integration_cases/test_raft_flow_control.rs

+
+    /*
+    1: cap=0/start=0/count=0/buffer=[]
+    2: cap=256/start=0/count=1/buffer=[2]


count should be 1?

I am not sure why MsgPropose sends 2 to others, but it did.

hicqu · 2021-07-20T12:26:43Z

harness/tests/integration_cases/test_raft_flow_control.rs

+    resp.index = r.raft_log.last_index();
+    r.step(resp).unwrap();
+
+    for (&id, pr) in r.prs().iter() {


The loop is not necessary if only tests id == 2.

jayzhan211 · 2021-07-20T12:32:38Z

I will use buffer.capacity, not cap(), because I found that we initialize buffer with cap

jayzhan211 · 2021-07-20T12:48:17Z

@hicqu PTAL

hicqu · 2021-07-21T05:36:59Z

@jayzhan211 seems some commits break the DCO check. Could you rebase and squash the PR like this:

git checkout free -b backup-free
git branch -D free
git checkout master -b free
git merge backup-free --no-commits --no-logs --squash
git commit -s     # then cleanup squashed commit logs

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

BusyJay

@hicqu you may need a method to monitor the memory correctly.

BusyJay · 2021-07-21T06:55:14Z

src/tracker/inflights.rs

+
+    /// Number of inflight messages.
+    #[inline]
+    pub fn count(&self) -> usize {


How about making it doc hiddern as it's only used in tests?

hicqu · 2021-07-21T07:23:15Z

you may need a method to monitor the memory correctly.

@BusyJay Do you mean we need fn Raft::inflight_buffers_size?

jayzhan211 · 2021-07-21T08:26:07Z

you may need a method to monitor the memory correctly.

@BusyJay Do you mean we need fn Raft::inflight_buffers_size?

Add a function that return buffer.capacity()?

BusyJay · 2021-07-22T05:51:09Z

@BusyJay Do you mean we need fn Raft::inflight_buffers_size?

Yes, something like that.

jayzhan211 · 2021-07-22T06:04:23Z

the buffer size is count, the buffer capacity is either 0 or cap

hicqu · 2021-07-22T11:12:18Z

@jayzhan211 we need to add a new method for Raft:

+
+    /// Get the inflight buffer size.
+    pub fn inflight_buffers_size(&self) -> usize {
+        let mut total_size = 0;
+        for (_, pr) in self.prs().iter() {
+            total_size += pr.ins.buffer.capacity();
+        }
+        total_size
+    }

to get the allocated buffer size for a raft peer.

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

BusyJay

Thanks!

jayzhan211 · 2021-07-23T06:59:43Z

Thanks! @hicqu @BusyJay

jayzhan211 commented Mar 18, 2021

View reviewed changes

BusyJay reviewed Mar 18, 2021

View reviewed changes

jayzhan211 marked this pull request as draft March 18, 2021 09:10

jayzhan211 marked this pull request as ready for review March 26, 2021 04:39

jayzhan211 requested a review from BusyJay March 26, 2021 07:58

jayzhan211 marked this pull request as draft March 27, 2021 01:21

jayzhan211 marked this pull request as ready for review March 27, 2021 01:30

BusyJay reviewed Apr 2, 2021

View reviewed changes

hicqu mentioned this pull request Jul 1, 2021

introduce elastic inflight to save memory #448

Closed

BusyJay reviewed Jul 5, 2021

View reviewed changes

jayzhan211 commented Jul 9, 2021

View reviewed changes

hicqu added a commit to hicqu/raft-rs that referenced this pull request Jul 19, 2021

implement tikv#423 (comment)

837023d

Signed-off-by: qupeng <qupeng@pingcap.com>

jayzhan211 pushed a commit to jayzhan211/raft-rs that referenced this pull request Jul 20, 2021

implement tikv#423 (comment)

09db393

Signed-off-by: qupeng <qupeng@pingcap.com>

hicqu previously approved these changes Jul 20, 2021

View reviewed changes

BusyJay reviewed Jul 20, 2021

View reviewed changes

jayzhan211 dismissed hicqu’s stale review via edd3d26 July 20, 2021 12:19

hicqu reviewed Jul 20, 2021

View reviewed changes

hicqu previously approved these changes Jul 21, 2021

View reviewed changes

free memory for inflight

63ae9b1

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 dismissed hicqu’s stale review via 63ae9b1 July 21, 2021 06:27

jayzhan211 force-pushed the free branch from 9e4549b to 63ae9b1 Compare July 21, 2021 06:27

jayzhan211 requested review from hicqu and BusyJay July 21, 2021 06:30

BusyJay reviewed Jul 21, 2021

View reviewed changes

monitor raft peer capacity

9f0caf6

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 requested a review from BusyJay July 22, 2021 12:42

hicqu approved these changes Jul 23, 2021

View reviewed changes

BusyJay approved these changes Jul 23, 2021

View reviewed changes

hicqu merged commit fa0a7c8 into tikv:master Jul 23, 2021

Add free() for raft node #423

Add free() for raft node #423

Conversation

jayzhan211 commented Mar 18, 2021 • edited by hicqu

jayzhan211 commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay commented Mar 18, 2021

Choose a reason for hiding this comment

jayzhan211 Apr 2, 2021 • edited

Choose a reason for hiding this comment

jayzhan211 Apr 2, 2021 • edited

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

hicqu commented Jun 28, 2021

hicqu commented Jun 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu commented Jul 5, 2021 • edited

hicqu commented Jul 9, 2021

jayzhan211 commented Jul 9, 2021

BusyJay commented Jul 9, 2021

jayzhan211 commented Jul 9, 2021

jayzhan211 Jul 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Jul 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu commented Jul 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Jul 20, 2021 • edited

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

jayzhan211 commented Jul 20, 2021

jayzhan211 commented Jul 20, 2021

hicqu commented Jul 21, 2021

BusyJay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu commented Jul 21, 2021 • edited

jayzhan211 commented Jul 21, 2021

BusyJay commented Jul 22, 2021

jayzhan211 commented Jul 22, 2021 • edited

hicqu commented Jul 22, 2021

BusyJay left a comment

Choose a reason for hiding this comment

jayzhan211 commented Jul 23, 2021

jayzhan211 commented Mar 18, 2021 •

edited by hicqu

jayzhan211 Apr 2, 2021 •

edited

jayzhan211 Apr 2, 2021 •

edited

hicqu commented Jul 5, 2021 •

edited

jayzhan211 Jul 9, 2021 •

edited

jayzhan211 Jul 19, 2021 •

edited

jayzhan211 Jul 20, 2021 •

edited

hicqu commented Jul 21, 2021 •

edited

jayzhan211 commented Jul 22, 2021 •

edited