Load balancer

[Philip-NLnetLabs]

Load balancer for client transports

[bal-e]

This is a really cool feature! My main concern is the complexity of the most important functions. I'd also like to see more explicit .expect() messages that explain underlying assumptions.

[bal-e]

Prefer to use rand::Rng::gen_bool()?

[Philip-NLnetLabs]

gen_bool is a method. Is it worth first selecting a random number generator to be able to invoke the method? Or is there a shortcut?

[tertsdiepraam]

I think the shortest is

use rand::{thread_rng, Rng};
thread_rng().gen_bool(PROBE_P)

[Philip-NLnetLabs]

Is that better than the existing code?

[tertsdiepraam]

That's for you and @bal-e to fight over :)

I don't mind either way. I just wanted to see what it would become.

[bal-e]

gen_bool() does the same thing you're doing right now, @Philip-NLnetLabs, it's just more explicit. There's no performance change or anything.

[Philip-NLnetLabs]

If gen_bool would be a function, just like random I would agree. But now suddenly we introduce a lot more machinery. It doesn't actually improve the code. But now the reader has to know what thread_rng is and if that is the right choice.

If is even worse, the reader now has to read the description of gen_bool to figure out what is does and when it panics.

I want code to be as simple as possible. And in this case the change seems increase code complexity not decrease it.

[bal-e]

I disagree with "the reader now has to read the description of gen_bool to figure out what is does and when it panics." gen_bool is a far more descriptive name ("generate boolean") than the current expression. You are implying that readers would understand random better than they would understand gen_bool. What about random's panics? Which random number generator does it use? What range of numbers does it produce? Those are all non-obvious questions that the use of random raises here.

[bal-e]

Instead of cloning the whole Vec and then filtering out of it, why not use .filter().collect()?

[Philip-NLnetLabs]

That would require two loops: one to update queue_length, burst_start and burst, and another for the filter. Do you think it is worth it?

[bal-e]

Updating those parameters can be done in forward order too, right? Then you can do them using .map() or .inspect() in the same iterator chain.

[Philip-NLnetLabs]

I'm confused. The current loop updates conn_stats. How would that work with map?

[bal-e]

Well, it only updates conn_stats at corresponding indices. You can .zip(&mut conn_stats) or use .enumerate() to get the right index to use.

[bal-e]

Here's the full code:

let tmp_conn_rt: Vec<_> = conn_rt
    .iter()
    .cloned()
    .zip(&mut conn_stats)
    .filter_map(|(mut conn, stats)| {
        // Fill-in current queue length.
        conn.queue_length = Arc::strong_count(&stats.queue_length_plus_one) - 1;
        if let Some(max_burst) = stats.max_burst {
            if stats.burst_start.elapsed() > stats.burst_interval {
                stats.burst_start = Instant::now();
                stats.burst = 0;
            }
            if stats.burst > max_burst {
                // Filter out the connection.
                return None;
            }
        }
        Some(conn)
    })
    .collect();

[Philip-NLnetLabs]

I don't see how this is more readable.

[bal-e]

It's more efficient and more idiomatic. Readers don't need to consider how entries get filtered using swap_remove(), nor do they have to think about the backward iteration. Instead of indexing conn_stats, we just naturally get the right element from it to work with.