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

Forwarder: clean up packet_vec filter #30921

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

apfitzge
Copy link
Contributor

@apfitzge apfitzge commented Mar 27, 2023

Problem

Saw an odd use of filter_map

Summary of Changes

Use chained methods to do equivalent operations in less code

Fixes #

@apfitzge apfitzge marked this pull request as ready for review March 28, 2023 16:56
None
}
})
.filter(|p| !p.meta().forwarded())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new code is cleaner definitely. Isn't the left side more efficient though without multiple loops?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't do multiple loops, iterators are a zero-cost abstraction: https://doc.rust-lang.org/book/ch13-04-performance.html

Closures and iterators are Rust features inspired by functional programming language ideas. They contribute to Rust’s capability to clearly express high-level ideas at low-level performance. The implementations of closures and iterators are such that runtime performance is not affected. This is part of Rust’s goal to strive to provide zero-cost abstractions.

They get compiled directly, so expansion isn't really a thing, but we kind of think how expanding these two versions might work...
Roughly the code before was:

    let mut packet_vec = vec![];
    for p in forwardable_packets {
        if !p.meta().forwarded() && self.data_budget.take(p.meta().size) {
            if let Some(data) = p.data(..) {
                packet_vec.push(data.to_vec());
            }
        }
    }

And after the change, (again roughly):

    let mut packet_vec = vec![];
    for p in forwardable_packets {
        if p.meta().forwarded() { continue; }
        if !p.data_budget.take(p.meta().size) { continue; }
        if let Some(data) = p.data(..) {
            packet_vec.push(data.to_vec());
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know! Thanks

Copy link
Contributor

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #30921 (bbacf68) into master (b53656b) will decrease coverage by 0.1%.
The diff coverage is 0.0%.

@@            Coverage Diff            @@
##           master   #30921     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         727      727             
  Lines      205118   205116      -2     
=========================================
- Hits       167256   167246     -10     
- Misses      37862    37870      +8     

@apfitzge apfitzge merged commit b72be0f into solana-labs:master Mar 28, 2023
@apfitzge apfitzge deleted the refactor/forwarder-2 branch March 29, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants