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

Benches for reloadable collectors #1090

Merged
merged 8 commits into from
Nov 23, 2020

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Nov 5, 2020

Motivation

The docs state that using the reload module inflicts a performance penalty but there was no further data about how big the penalty is. This PR adds _reload variants to the filter benchmarks. Results are a bit inconclusive but the tl;dr is "yes, there is a performance impact and yes, in some cases it's significant. Generally the more complex the collector the less it matters.".

The benchmarks after this PR are a bit noisy; happy to rework this but wasn't sure what the preferred way was. There's already a fair bit of duplication in the benchmarks but this PR isn't helping with that.

@dvdplm dvdplm requested review from hawkw and a team as code owners November 5, 2020 12:15
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Thanks for this!

It's definitely on my radar to look into potential optimizations for reload's common case performance (i.e., when the subscriber isn't currently being reloaded) — maybe a sharded lock is worth trying?

It looks like rustfmt checks are failing on this branch. Mind running rustfmt?

@dvdplm
Copy link
Contributor Author

dvdplm commented Nov 5, 2020

It looks like rustfmt checks are failing on this branch. Mind running rustfmt?

Done. I keep forgetting that, very annoying.

@dvdplm
Copy link
Contributor Author

dvdplm commented Nov 5, 2020

maybe a sharded lock is worth trying?

I did a quick test with parking_lot locks just to see what kind of change pops up in the benches. It is a substantial improvement, e.g.:

static/baseline_single_threaded                                                                            
                        time:   [73.932 ns 73.954 ns 73.976 ns]
                        change: [+0.0673% +0.1126% +0.1545%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild
static/single_threaded  time:   [37.470 ns 37.500 ns 37.529 ns]                                    
                        change: [+0.6040% +0.6925% +0.7867%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
  5 (5.00%) high mild
static/single_threaded_reload                                                                             
                        time:   [48.750 ns 48.754 ns 48.759 ns]
                        change: [-26.931% -26.903% -26.877%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  3 (3.00%) high mild
  6 (6.00%) high severe

@hawkw
Copy link
Member

hawkw commented Nov 6, 2020

Mm, that's pretty unsurprising. In general, I'd recommend users who are more concerned with performance than with their number of dependencies to enable the parking_lot feature...

@dvdplm
Copy link
Contributor Author

dvdplm commented Nov 6, 2020

Mm, that's pretty unsurprising. In general, I'd recommend users who are more concerned with performance than with their number of dependencies to enable the parking_lot feature...

Should I try to solidify that parking_lot experiment into something more solid? Or do you have a preference for the shared lock from crossbeam? Given that parking_lot is a common dependency alongside tracing (where it's already optional) it might be more attractive than another lock.

@dvdplm dvdplm requested a review from hawkw November 23, 2020 10:04
The warnings about dead code in the `support` module are 'fixed' here by overriding Clippy which is pretty sad. Not sure what to do about it though other than moving the support code to its own crate (overkill?).
@hawkw hawkw merged commit 302d4a9 into tokio-rs:master Nov 23, 2020
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