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

Performance metrics for NetworkManager future #6746

Merged
merged 10 commits into from
Feb 29, 2024

Conversation

emhane
Copy link
Member

@emhane emhane commented Feb 23, 2024

Closes #6745.

  • Adds Gauges for NetworkManager future and its nested items, to measure the accumulated time spent in the future on a whole and in each nested item.
  • Moves SwarmEvent processing to separate function to improve readability of the new code.

@emhane emhane added C-perf A change motivated by improving speed, memory usage or disk footprint A-observability Related to tracing, metrics, logs and other observability tools A-networking Related to networking in general labels Feb 23, 2024
crates/net/network/src/manager.rs Outdated Show resolved Hide resolved
crates/net/network/src/manager.rs Show resolved Hide resolved
error!("Network message channel closed.");
return Poll::Ready(())
let acc = &mut poll_durations.acc_network_handle;
duration_metered_exec!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need the macro for those two calls?

with macro this is more verbose than without

we can also get rid of of the second Instant::now if we reuse the previous network handle duration to offset the elpased

Copy link
Member Author

Choose a reason for hiding this comment

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

it's favourable here to be as exact as possible with the start time

Copy link
Collaborator

Choose a reason for hiding this comment

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

I find these macros very hard to read, and this adds more code than without the macro.
since we're only using this twice I'd prefer using instants directly, which is just 2 lines and no additional scope

Copy link
Member Author

@emhane emhane Feb 27, 2024

Choose a reason for hiding this comment

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

trying to be consistent with pattern introduced in #6688

Copy link
Collaborator

Choose a reason for hiding this comment

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

in this case, not using a macro makes it easier to read, less code and no additional scope.
and we can also save 1 syscall if we reuse the first instant.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, I'm not quite sure which instant you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can do:

let now = Instant::now();

...
poll_durations.acc_network_handle = now.elpased();


...

poll_durations.acc_swarm = now.elpased() - poll_durations.acc_network_handle;

@emhane emhane requested a review from mattsse February 27, 2024 01:13
error!("Network message channel closed.");
return Poll::Ready(())
let acc = &mut poll_durations.acc_network_handle;
duration_metered_exec!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

in this case, not using a macro makes it easier to read, less code and no additional scope.
and we can also save 1 syscall if we reuse the first instant.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

@emhane emhane added this pull request to the merge queue Feb 29, 2024
Merged via the queue into main with commit 559124a Feb 29, 2024
29 checks passed
@emhane emhane deleted the emhane/poll-duration-network-manager branch February 29, 2024 03:05
fgimenez pushed a commit to fgimenez/reth that referenced this pull request Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general A-observability Related to tracing, metrics, logs and other observability tools C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance metrics for NetworkManager future
3 participants