Out of line ProgOption::SetProperty for int and std::string

[ktf]

The specializations are common enough to show up in O2 compilation profiles and they are not time critical (once per run at max).

[ktf]

@rbx @dennisklein I have something similar for EventManager which also suffer from long compilation times. What do you think?

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request modifies the ProgOptions class in the fair::mq namespace, specifically focusing on the SetProperty method. The changes involve adding template specializations for std::string and int types, relocating the method implementation outside the class definition, and introducing extern template declarations. These modifications enhance the separation of declaration and implementation, improve thread safety, and maintain the core functionality of setting and managing configuration properties.

Changes

File	Change Summary
fairmq/ProgOptions.h	- Updated SetProperty method signature - Added extern template declarations for int and std::string types
fairmq/ProgOptions.cxx	- Added template specializations for SetProperty method for std::string and int types

Sequence Diagram

sequenceDiagram
    participant Client
    participant ProgOptions
    participant Mutex
    participant VariableMap
    participant EventEmitter

    Client->>ProgOptions: SetProperty(key, value)
    ProgOptions->>Mutex: Acquire unique lock
    ProgOptions->>VariableMap: Set value for key
    ProgOptions->>EventEmitter: Emit property change event
    Mutex->>ProgOptions: Release lock

Loading

The sequence diagram illustrates the thread-safe process of setting a property, which involves acquiring a mutex, updating the variable map, emitting an event, and then releasing the mutex.

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7722237 and e6aad7b.

📒 Files selected for processing (2)

• fairmq/ProgOptions.cxx (1 hunks)
• fairmq/ProgOptions.h (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• fairmq/ProgOptions.cxx

🔇 Additional comments (3)

fairmq/ProgOptions.h (3)

132-132: LGTM! Clean template method declaration.

The declaration is minimal and follows good template design practices.

---

268-279: LGTM! Thread-safe implementation with proper event handling.

The implementation demonstrates good practices:

• Uses RAII lock guard for thread safety
• Properly unlocks before event emission to prevent potential deadlocks
• Correctly handles type decay for reference types

---

281-282: LGTM! Strategic use of extern templates to reduce compilation overhead.

The extern template declarations for int and std::string types align with the PR objectives to reduce compilation time by preventing implicit instantiation in each translation unit.

Let's verify the impact on compilation time with the following script:

✅ Verification successful

Verified: Extern templates will effectively reduce compilation overhead

The analysis confirms that these extern templates will have a meaningful impact on compilation time:

• Found 13 translation units that would otherwise each generate their own template instantiations
• Proper implementation with declarations in header and definitions in source file
• Widespread usage across tests and examples validates the choice of int/string specializations

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find translation units that might be affected by the template instantiation
# to validate the potential compilation time improvement.

# Look for files that include this header and use SetProperty
rg -l "SetProperty.*[<(](?:int|string|std::string)" --type cpp

# Look for the explicit template instantiation in the source file
rg "template.*ProgOptions::SetProperty.*[<(](?:int|std::string)" --type cpp

Length of output: 1025

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[ktf]

For the record, there seems to be some issues with the testing infrastructure at GSI.

[rbx]

Looks like it could be a nice optimization for the compilation speed for you. How much does it bring (just curious)?

[dennisklein]

For the record, there seems to be some issues with the testing infrastructure at GSI.

The Jenkinsfile in master branch is outdated so the CI failed. Safer to open against dev branch :)

[ktf]

I opened it against whatever branch it gave me by default. I will reopen against dev.

Tests aside, would you be ok with such change? Can it be backported to the version which we use in ALICE?

[ktf]

In limited tests, the whole FairMQ compilation which happens inside O2 (i.e. the stuff which is inlined in templates) accounts for 10% of the framework compilation time (roughly 70/700 seconds, including unit tests and test workflows). This particular one seems to save ~5s on my M1 mac.

[ktf]

(to be clear, the single compilation is 100ms, the problem is that it's done in each single test / workflow)

[ktf]

Profiled via -ftime-trace and ClangBuildAnalyzer.

[ktf]

I am both amazed and scared about the coderabbit thing...