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

enha: add flamegraph feature flag #1913

Merged

Conversation

marcospb19-cw
Copy link
Contributor

@marcospb19-cw marcospb19-cw commented Dec 12, 2024

User description

to ease profiling by improving readability of generated graphs


PR Type

Enhancement


Description

  • Added a new 'flamegraph' feature flag to improve profiling and readability of generated graphs
  • Modified thread naming logic in src/config.rs to use simplified names when the 'flamegraph' feature is enabled:
    • Async threads are now named 'tokio-async' instead of 'tokio-async-{id}'
    • Blocking threads are now named 'tokio-blocking' instead of 'tokio-blocking-{id}'
  • Updated Cargo.toml to include the new 'flamegraph' feature flag
  • These changes will make it easier to analyze performance bottlenecks in flamegraphs by reducing the number of unique thread names

Changes walkthrough 📝

Relevant files
Enhancement
config.rs
Implement conditional thread naming for flamegraph feature

src/config.rs

  • Added conditional formatting for thread names based on the
    'flamegraph' feature flag
  • Modified async and blocking thread naming logic to improve flamegraph
    readability
  • +10/-2   
    Configuration changes
    Cargo.toml
    Add flamegraph feature flag to Cargo.toml                               

    Cargo.toml

    • Added a new feature flag 'flamegraph' to help generate flamegraphs
    +3/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    to ease profiling by improving readability of generated graphs
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication
    The conditional logic for thread naming is repeated for both async and blocking threads. Consider refactoring to reduce duplication and improve maintainability.

    Tracing Instrumentation
    The thread naming function is not instrumented with tracing. Consider adding tracing to log thread creation and naming for better observability.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Ensure consistent return mechanisms throughout the function for better code clarity and maintainability

    Ensure consistent return types across all code paths by using the same return
    mechanism (either return or expression-based return) throughout the function.

    src/config.rs [152-165]

    -if cfg!(feature = "flamegraph") {
    -    return format!("tokio-async");
    +let thread_name = if cfg!(feature = "flamegraph") {
    +    "tokio-async".to_string()
     } else {
    -    return format!("tokio-async-{}", async_id);
    -}
    +    format!("tokio-async-{}", async_id)
    +};
    +return thread_name;
     
     // ... (other code)
     
    -if cfg!(feature = "flamegraph") {
    -    format!("tokio-blocking")
    +let thread_name = if cfg!(feature = "flamegraph") {
    +    "tokio-blocking".to_string()
     } else {
         format!("tokio-blocking-{}", blocking_id)
    -}
    +};
    +thread_name
    Suggestion importance[1-10]: 7

    Why: This suggestion addresses an important consistency issue in the code. It proposes a unified approach to returning values, which can significantly improve code clarity and maintainability.

    7
    Improve code structure and readability by using a match expression for conditional logic

    Consider using a match expression instead of nested if statements for better
    readability and to handle all possible cases explicitly.

    src/config.rs [152-156]

    -if cfg!(feature = "flamegraph") {
    -    return format!("tokio-async");
    -} else {
    -    return format!("tokio-async-{}", async_id);
    +match cfg!(feature = "flamegraph") {
    +    true => "tokio-async".to_string(),
    +    false => format!("tokio-async-{}", async_id),
     }
    Suggestion importance[1-10]: 5

    Why: The suggestion to use a match expression instead of if-else is valid and can improve readability. However, the impact is moderate as the current code is already functional and clear.

    5

    @gabriel-aranha-cw
    Copy link
    Contributor

    /flamegraph

    @gabriel-aranha-cw
    Copy link
    Contributor

    Flamegraphs:
    Run ID: bench-105845476

    Git Info:

    Flamegraphs:
    idle:

    write:

    read:

    @gabriel-aranha-cw gabriel-aranha-cw enabled auto-merge (squash) December 16, 2024 21:48
    @gabriel-aranha-cw gabriel-aranha-cw merged commit 17aacb7 into main Dec 16, 2024
    34 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the enha-add-flamegraph-feature-flag-to-ease-profiling branch December 16, 2024 21:56
    @gabriel-aranha-cw
    Copy link
    Contributor

    gabriel-aranha-cw commented Dec 16, 2024

    Final benchmark:
    Run ID: bench-4066667646

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1157.00, Min: 668.00, Avg: 988.24, StdDev: 63.25
    TPS Stats: Max: 1136.00, Min: 797.00, Avg: 958.36, StdDev: 65.57

    Plot: View Plot

    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