very dumb leader selection #425

aeyakovenko · 2018-06-23T23:09:59Z

very dumb leader selection (fixes part 3 of Make validators restartable #299)

rob-solana · 2018-06-23T23:42:25Z

src/bin/fullnode.rs

+        "t",
+        "",
+        "testnet; connec to the network at this gossip entry point",
+        "host:port",


recommend HOST:PORT

rob-solana · 2018-06-23T23:43:05Z

src/bin/fullnode.rs

+    opts.optopt(
+        "t",
+        "",
+        "testnet; connec to the network at this gossip entry point",


s/connec to/connect to at gossip entry point HOST:PORT

rob-solana

nits, otherwise LGTM

garious

"When in Rome" please. Using full words and no one letter prefixes would be much appreciated.

garious · 2018-06-23T23:29:38Z

src/bin/fullnode.rs

+    opts.optopt(
+        "t",
+        "",
+        "testnet; connec to the network at this gossip entry point",


garious · 2018-06-23T23:32:11Z

src/crdt.rs

        blob_sender.send(q)?;
        Ok(())
    }
+    /// FIXME: This is obviously the wrong way to do this. Need to implement leader selection


Let's consistently used TODO or FIXME. Codebase currently uses TODO. If you prefer FIXME, can you change the others?

I thought FIXME was a higher priority TODO :)

garious · 2018-06-23T23:36:46Z

src/bin/fullnode.rs

-        let file = File::open(path.clone()).expect(&format!("file not found: {}", path));
-        let leader = serde_json::from_reader(file).expect("parse");
+        let taddr = testnet.parse().unwrap();
+        let entry = ReplicatedData::new_entry_point(taddr);


The term entry is already actively used throughout the codebase. Can you find another?

garious · 2018-06-23T23:37:19Z

src/bin/fullnode.rs

        );
-        let file = File::open(path.clone()).expect(&format!("file not found: {}", path));
-        let leader = serde_json::from_reader(file).expect("parse");
+        let taddr = testnet.parse().unwrap();


testnet_addr?

garious · 2018-06-23T23:38:29Z

src/crdt.rs

+    }
+
+    /// FIXME: This is obviously the wrong way to do this. Need to implement leader selection
+    /// A t-shirt for the first person to actually use this bad behavior to attack the alpha testnet


Seems fair :)

garious · 2018-06-23T23:40:42Z

src/crdt.rs

+    /// FIXME: This is obviously the wrong way to do this. Need to implement leader selection
+    /// A t-shirt for the first person to actually use this bad behavior to attack the alpha testnet
+    fn update_leader(&mut self) {
+        if let Some(lid) = self.top_leader() {


id would be a better name since lid is a word. If not specific enough, use leader_id

garious · 2018-06-23T23:42:21Z

src/crdt.rs

+    fn test_update_leader() {
+        logger::setup();
+        let me = ReplicatedData::new_leader(&"127.0.0.1:1234".parse().unwrap());
+        let lead = ReplicatedData::new_leader(&"127.0.0.1:1234".parse().unwrap());


leader0 and leader1 would be consistent with naming elsewhere

garious · 2018-06-23T23:42:49Z

src/crdt.rs

+        let lead = ReplicatedData::new_leader(&"127.0.0.1:1234".parse().unwrap());
+        let lead2 = ReplicatedData::new_leader(&"127.0.0.1:1234".parse().unwrap());
+        let mut crdt = Crdt::new(me.clone());
+        assert_matches!(crdt.top_leader(), None);


assert_eq

garious · 2018-06-23T23:45:30Z

src/crdt.rs

+                let _ = obj.write().unwrap().update_leader();
+                let elapsed = timestamp() - start;
+                if GOSSIP_SLEEP_MILLIS > elapsed {
+                    let left = GOSSIP_SLEEP_MILLIS - elapsed;


left is unnecessarily ambiguous. time_left or remaining would be better.

garious · 2018-06-23T23:46:43Z

src/crdt.rs

+        }
+        let mut sorted: Vec<_> = table.iter().collect();
+        sorted.sort_by_key(|a| a.1);
+        sorted.last().map(|a| *(*(*a).0))


Can that be simplified?

hehe, its borrow checker all the way down. I am not sure it can be without copying the generic array key. and there is no way to sort an iterator.

I was wrong

aeyakovenko · 2018-06-24T14:14:21Z

@garious still blocked?

garious · 2018-06-24T15:25:27Z

@aeyakovenko, if you're happy with #427, can you merge it, rebase this PR on it, and remove that #[ignore] on the drone test?

@aeyakovenko

@aeyakovenko, this one failed on Rust stable.

garious · 2018-06-24T17:18:24Z

Merged in #430

Bumps [eslint](https://github.com/eslint/eslint) from 7.7.0 to 7.8.1. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/master/CHANGELOG.md) - [Commits](eslint/eslint@v7.7.0...v7.8.1) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

… (#425) implements weighted shuffle using binary tree (#185) This is partial port of firedancer's implementation of weighted shuffle: https://github.com/firedancer-io/firedancer/blob/3401bfc26/src/ballet/wsample/fd_wsample.c Though Fenwick trees use less space, inverse queries require an additional O(log n) factor for binary search resulting an overall O(n log n log n) performance for weighted shuffle. This commit instead uses a binary tree where each node contains the sum of all weights in its left sub-tree. The weights themselves are implicitly stored at the leaves. Inverse queries and updates to the tree all can be done O(log n) resulting an overall O(n log n) weighted shuffle implementation. Based on benchmarks, this results in 24% improvement in WeightedShuffle::shuffle: Fenwick tree: test bench_weighted_shuffle_new ... bench: 36,686 ns/iter (+/- 191) test bench_weighted_shuffle_shuffle ... bench: 342,625 ns/iter (+/- 4,067) Binary tree: test bench_weighted_shuffle_new ... bench: 59,131 ns/iter (+/- 362) test bench_weighted_shuffle_shuffle ... bench: 260,194 ns/iter (+/- 11,195) Though WeightedShuffle::new is now slower, it generally can be cached and reused as in Turbine: https://github.com/anza-xyz/agave/blob/b3fd87fe8/turbine/src/cluster_nodes.rs#L68 Additionally the new code has better asymptotic performance. For example with 20_000 weights WeightedShuffle::shuffle is 31% faster: Fenwick tree: test bench_weighted_shuffle_new ... bench: 255,071 ns/iter (+/- 9,591) test bench_weighted_shuffle_shuffle ... bench: 2,466,058 ns/iter (+/- 9,873) Binary tree: test bench_weighted_shuffle_new ... bench: 830,727 ns/iter (+/- 10,210) test bench_weighted_shuffle_shuffle ... bench: 1,696,160 ns/iter (+/- 75,271) (cherry picked from commit b6d2237) Co-authored-by: behzad nouri <[email protected]>

…ana-labs#185) (solana-labs#425) implements weighted shuffle using binary tree (solana-labs#185) This is partial port of firedancer's implementation of weighted shuffle: https://github.com/firedancer-io/firedancer/blob/3401bfc26/src/ballet/wsample/fd_wsample.c Though Fenwick trees use less space, inverse queries require an additional O(log n) factor for binary search resulting an overall O(n log n log n) performance for weighted shuffle. This commit instead uses a binary tree where each node contains the sum of all weights in its left sub-tree. The weights themselves are implicitly stored at the leaves. Inverse queries and updates to the tree all can be done O(log n) resulting an overall O(n log n) weighted shuffle implementation. Based on benchmarks, this results in 24% improvement in WeightedShuffle::shuffle: Fenwick tree: test bench_weighted_shuffle_new ... bench: 36,686 ns/iter (+/- 191) test bench_weighted_shuffle_shuffle ... bench: 342,625 ns/iter (+/- 4,067) Binary tree: test bench_weighted_shuffle_new ... bench: 59,131 ns/iter (+/- 362) test bench_weighted_shuffle_shuffle ... bench: 260,194 ns/iter (+/- 11,195) Though WeightedShuffle::new is now slower, it generally can be cached and reused as in Turbine: https://github.com/anza-xyz/agave/blob/b3fd87fe8/turbine/src/cluster_nodes.rs#L68 Additionally the new code has better asymptotic performance. For example with 20_000 weights WeightedShuffle::shuffle is 31% faster: Fenwick tree: test bench_weighted_shuffle_new ... bench: 255,071 ns/iter (+/- 9,591) test bench_weighted_shuffle_shuffle ... bench: 2,466,058 ns/iter (+/- 9,873) Binary tree: test bench_weighted_shuffle_new ... bench: 830,727 ns/iter (+/- 10,210) test bench_weighted_shuffle_shuffle ... bench: 1,696,160 ns/iter (+/- 75,271) (cherry picked from commit b6d2237) Co-authored-by: behzad nouri <[email protected]>

generic array fail case

43eaf84

aeyakovenko requested review from garious, rob-solana and sakridge June 23, 2018 23:09

aeyakovenko changed the title ~~generic array fail case~~ very dumb leader selection, and generic array fail case Jun 23, 2018

aeyakovenko added 2 commits June 23, 2018 16:30

tests

cabc9a8

get rid of dummy test

bef45b9

aeyakovenko changed the title ~~very dumb leader selection, and generic array fail case~~ very dumb leader selection Jun 23, 2018

fix logs

e41fcdd

rob-solana reviewed Jun 23, 2018

View reviewed changes

fix docs

bbdb8a2

rob-solana reviewed Jun 23, 2018

View reviewed changes

nits

a98f58b

garious reviewed Jun 23, 2018

View reviewed changes

aeyakovenko added 4 commits June 23, 2018 16:51

fixed!

1e2e0d0

comments

304fd21

remove hardcoded ports

b800a12

borrow checker

b68903c

aeyakovenko mentioned this pull request Jun 24, 2018

test_send_airdrop depends on server port order #426

Closed

disable test that depends on specific server ports

a286575

aeyakovenko referenced this pull request in garious/solana Jun 24, 2018

Disable another flakey test

9b5061c

@aeyakovenko, this one failed on Rust stable.

garious mentioned this pull request Jun 24, 2018

Naive leader selection #430

Merged

garious closed this Jun 24, 2018

very dumb leader selection #425

very dumb leader selection #425

Uh oh!

Conversation

aeyakovenko commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rob-solana left a comment

Choose a reason for hiding this comment

Uh oh!

garious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeyakovenko Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeyakovenko Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeyakovenko commented Jun 24, 2018

Uh oh!

garious commented Jun 24, 2018

Uh oh!

garious commented Jun 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aeyakovenko commented Jun 23, 2018 •

edited

Loading

aeyakovenko Jun 23, 2018 •

edited

Loading

aeyakovenko Jun 24, 2018 •

edited

Loading