Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

raft: avoid the unnecessary clone of the raft unstable log #2373

Open
siddontang opened this Issue Oct 11, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@siddontang
Copy link
Contributor

siddontang commented Oct 11, 2017

When we new the Raft Ready, we will clone all unstable logs, see:

        let mut rd = Ready {
            entries: raft.raft_log.unstable_entries().unwrap_or(&[]).to_vec(),
            messages: raft.msgs.drain(..).collect(),
            ..Default::default()
        };

But this is unnecessary, the unstable logs will not be changed until we commit the append ready.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 2, 2019

This code doesn't seem to exist anymore. Is this issue still relevant?

@siddontang

This comment has been minimized.

Copy link
Contributor Author

siddontang commented Feb 2, 2019

yes, @brson

It still exists in the raft-rs library.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 6, 2019

I must have been grepping the wrong repo. Here it is: https://github.com/pingcap/raft-rs/blob/3eb423597d6650d3dbe4346a47ca12c6022fd154/src/raw_node.rs#L130

This seems like it should be relatively easy for someone with some understanding of the crate, but personally it's not obvious to me how to do it from a glance. More detail would help a contributor. As a naive contributor I'd start by just removing the entries field from Ready and seeing what happens.

@brson brson added the D: Easy label Feb 19, 2019

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 19, 2019

To be perfectly clear, the problem is the call to to_vec here:

raft.raft_log.unstable_entries().unwrap_or(&[]).to_vec(),

It might copy a lot of data for no reason. This task is to figure out how to delay that copy until it is definitely needed.

@liyuntao

This comment has been minimized.

Copy link

liyuntao commented Mar 6, 2019

To get rid of copy, is it possible to change the entries field's type to &[Entry] in Ready struct?

However this would lead to first mutable borrow on raw_node ends later than second mutable borrow, thus introduce some compile time error I can't solved by myself.

error[E0499]: cannot borrow `raw_node` as mutable more than once at a time
   --> tests/integration_cases/test_raw_node.rs:455:5
    |
453 |     let rd = raw_node.ready();
    |              -------- first mutable borrow occurs here
454 |     assert!(cmp_ready(&rd, &None, &None, &[], entries.clone(), false));
455 |     raw_node.advance(rd);
    |     ^^^^^^^^         -- first borrow later used here
    |     |
    |     second mutable borrow occurs here

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Mar 12, 2019

Thanks for giving it a look @liyuntao.

It seems that simply storing a slice for Ready::entries won't work, because that borrows from the RawNode; and RawNode::advance needs to be called mutably with the Ready that is already mutably borrowing it.

My inclination on seeing that is to not store 'entries' in Ready at all, but to just store an index or other value that will allow entries to be retrieved later, when 'advance' is called. I note that the 'advance' method doesn't even use the entries array except to grab the index of the very last entry, then advances the raft log with it.

Sadly though, Ready has an 'entries' method to access the Entry array, and it's used in a number of examples and test cases.

A common construct in the test cases shows all these APIs:

    let s = new_storage();
    let mut raw_node = new_raw_node(1, vec![], 10, 1, s.clone(), vec![new_peer(1)]);
    let rd = raw_node.ready();
    s.wl().append(rd.entries()).expect("");
    raw_node.advance(rd);

So to get rid of the extra clone of unstable entries one has to have a way to feed those entries to Storage::append and also to advance the log in raw_node.advance.

It seems like accomplishing this will require changes to APIs that I don't have a good understanding of. Maybe @Hoverbear has some insight.

@siddontang

This comment has been minimized.

Copy link
Contributor Author

siddontang commented Mar 13, 2019

/cc @BusyJay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.