perf: Add ScopedProfiler and ProfilerReporter to simplify profiling. #2080
Open
SharafMohamed wants to merge 179 commits intoy-scope:mainfrom
Open
perf: Add ScopedProfiler and ProfilerReporter to simplify profiling. #2080SharafMohamed wants to merge 179 commits intoy-scope:mainfrom
ScopedProfiler and ProfilerReporter to simplify profiling. #2080SharafMohamed wants to merge 179 commits intoy-scope:mainfrom
Conversation
…esting code for DP algo; Add final unit-test for testing PR end-to-end; Still lots of edge cases and sub-steps that could be tested more rigorously.
…e now stored logtype as it will no longer dangle.
…e used wherever possible.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors Profiler to separate runtime (named unordered_map) and compile-time (enum-indexed vector) measurements; adds RAII utilities ProfilerReporter and ScopedProfiler; Stopwatch now tracks call counts; instrumentation switched to PROFILE_SCOPE; tests and CMake updated (adds PROF_TEST_ENABLED). Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant PR as ProfilerReporter
participant SP as ScopedProfiler
participant Prof as Profiler
participant SW as Stopwatch
App->>PR: create ProfilerReporter
App->>SP: enter PROFILE_SCOPE(name)
SP->>Prof: start_runtime_measurement(name)
Prof->>SW: create/start stopwatch for name
Note over SW: elapsed time accumulates
App->>SP: exit scope
SP->>Prof: stop_runtime_measurement(name)
Prof->>SW: record elapsed / increment call_count
Prof->>Prof: update m_runtime_measurements[name]
App->>PR: destructor invoked (scope exit)
PR->>Prof: get_runtime_measurements()
Prof-->>PR: return measurements map
PR->>PR: format or copy into sink
PR-->>App: report measurements (log or sink)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/core/src/clp/clo/clo.cpp`:
- Around line 575-576: ProfilerReporter is instantiated but no profiling scopes
are used, so it will emit empty measurements; either add PROFILE_SCOPE macros
around meaningful functions (e.g., wrap the search and extract_ir bodies with
PROFILE_SCOPE("search") and PROFILE_SCOPE("extract_ir") respectively) so
ProfilerReporter records data, or remove/defer the ProfilerReporter
instantiation until instrumentation is added, or at minimum add a comment next
to the ProfilerReporter declaration explaining it’s intentional scaffolding;
update the ProfilerReporter usage or add PROFILE_SCOPE calls inside the
corresponding functions (search, extract_ir) to resolve the empty logs.
In `@components/core/src/clp/clp/FileCompressor.cpp`:
- Around line 31-32: Remove the unused using declaration for Profiler: delete
the line "using clp::Profiler;" from FileCompressor.cpp since only
ScopedProfiler (and macros like PROFILE_SCOPE) are used; confirm no remaining
references to Profiler in FileCompressor.cpp and run a build to ensure no symbol
was actually needed before committing.
In `@components/core/src/clp/clp/run.cpp`:
- Around line 18-19: Remove the unused using declaration for clp::Profiler since
Profiler is not referenced in this file and profiling is handled via the
PROFILE_SCOPE macro; keep any needed using clp::ProfilerReporter if it is used
elsewhere in this file (i.e., delete the line containing "using clp::Profiler;"
and leave "using clp::ProfilerReporter;" intact).
In `@components/core/src/clp/Profiler.hpp`:
- Around line 83-92: In check_runtime_timer_exists(std::string const& name) fix
the typo in the SPDLOG_ERROR message ("measurment" -> "measurement") and remove
the extraneous double semicolon after the return false statement so the function
reads a single "return false;" for the error path; verify the check uses
m_runtime_measurements.contains(name) and SPDLOG_ERROR still includes the timer
name for context.
In `@components/core/src/clp/ProfilerReporter.hpp`:
- Around line 20-31: Fix two typos in the class header comment for
ProfilerReporter: change "destructured" to "destructed" in the sentence about
object lifecycle and change "accidentaly" to "accidentally" in the sentence
about copy/move operations; update the comment block near the ProfilerReporter
description so it reads "Once the object is destructed (scope exit), the runtime
measurements are reported." and "prevent accidentally multiple reporting."
referencing the ProfilerReporter class in ProfilerReporter.hpp.
- Around line 1-2: The include guard macro in ProfilerReporter.hpp is
inconsistent: it currently uses CLP_PROFILER_REPORT_HPP; change it to
CLP_PROFILER_REPORTER_HPP everywhere (the `#ifndef/`#define at the top and the
matching `#endif` comment at the bottom) so the macro matches the file/class name
(ProfilerReporter) and avoids future collisions or confusion.
In `@components/core/src/clp/ScopedProfiler.hpp`:
- Around line 43-47: Replace the use of __LINE__ in the PROFILE_SCOPE macro with
a monotonic preprocessor counter to avoid name collisions when the macro is
expanded multiple times on the same source line: update the PROFILE_SCOPE
definition (which uses CLP_CONCAT_IMPL, CLP_CONCAT and the generated identifier
prefix __clp_profile_scope_) to use __COUNTER__ (or a guarded fallback) instead
of __LINE__ so each instantiation gets a unique variable name across the
translation unit.
In `@components/core/src/clp/Stopwatch.cpp`:
- Around line 24-26: Stopwatch::reset currently zeroes only m_time_taken but
leaves m_call_count unchanged; update Stopwatch::reset to also set m_call_count
= 0 so a direct call to Stopwatch::reset on a reused instance clears both time
and call count for consistency with Profiler::reset_runtime_measurements (which
rebuilds Stopwatches via clear()). Ensure you modify the reset() method in the
Stopwatch class to reset both m_time_taken and m_call_count.
In `@components/core/tests/test-ScopedProfiler.cpp`:
- Around line 10-12: Remove the duplicate using declaration for Profiler: in the
test file remove the repeated line "using clp::Profiler;" so that only a single
using clp::Profiler; remains (keep the existing using clp::ScopedProfiler; line
intact); this eliminates the redundant duplicate using statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 546f74f9-ad23-47e0-b624-660b7c74b74b
📒 Files selected for processing (15)
components/core/CMakeLists.txtcomponents/core/src/clp/Profiler.cppcomponents/core/src/clp/Profiler.hppcomponents/core/src/clp/ProfilerReporter.hppcomponents/core/src/clp/ScopedProfiler.hppcomponents/core/src/clp/Stopwatch.cppcomponents/core/src/clp/Stopwatch.hppcomponents/core/src/clp/clg/clg.cppcomponents/core/src/clp/clo/clo.cppcomponents/core/src/clp/clp/CMakeLists.txtcomponents/core/src/clp/clp/FileCompressor.cppcomponents/core/src/clp/clp/run.cppcomponents/core/tests/test-ProfilerReporter.cppcomponents/core/tests/test-ScopedProfiler.cppcomponents/core/tests/test-Stopwatch.cpp
SharafMohamed
changed the title
ScopedProfiler and ProfilerReporter for runtime measurements.
SharafMohamed
changed the title
ScopedProfiler and ProfilerReporter for runtime measurements. ScopedProfiler and ProfilerReporter to simplify profiling.
SharafMohamed
changed the title
ScopedProfiler and ProfilerReporter to simplify profiling. ScopedProfiler and ProfilerReporter to simplify profiling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/core/src/clp/Profiler.hpp`:
- Around line 61-71: The static init() in Profiler.hpp allocates
m_compile_time_measurements with new but never frees it; either document that
this allocation is intentional for process-lifetime usage or add a matching
cleanup path: implement a static deinit() that checks PROF_ACTIVE and
m_initialized, deletes the vector pointed to by m_compile_time_measurements,
sets the pointer to nullptr and clears m_initialized, and ensure callers invoke
deinit() (e.g., during shutdown) so m_compile_time_measurements is properly
freed.
- Around line 23-24: Fix the typo in the comment describing Measurement in
Profiler.hpp: change "fragemented" to "fragmented" in the sentence that begins
"A Measurement can be taken..." so the documentation reads "...accumulate
fragmented measurements into a single total run time." This is purely a comment
change near the Measurement description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6274c330-87d3-440e-bc65-adb4391f09f4
📒 Files selected for processing (5)
components/core/src/clp/Profiler.hppcomponents/core/src/clp/ProfilerReporter.hppcomponents/core/src/clp/ScopedProfiler.hppcomponents/core/tests/test-ProfilerReporter.cppcomponents/core/tests/test-ScopedProfiler.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/core/src/clp/clp/run.cpp (2)
162-162:⚠️ Potential issue | 🟡 MinorCoding guideline violation: use
false ==instead of!.As per coding guidelines, prefer
false == <expression>rather than!<expression>.📝 Proposed fix
- if (!compression_successful) { + if (false == compression_successful) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp/clp/run.cpp` at line 162, The condition uses negation operator on the boolean variable compression_successful; change the check to use the coding-guideline form by replacing the if (!compression_successful) test with if (false == compression_successful) (locate the occurrence in clp::run or the function containing compression_successful in run.cpp and update that conditional).
63-63:⚠️ Potential issue | 🟡 MinorCoding guideline violation: use
false ==instead of!.As per coding guidelines, prefer
false == <expression>rather than!<expression>.📝 Proposed fix
- if (!command_line_args.get_use_heuristic()) { + if (false == command_line_args.get_use_heuristic()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp/clp/run.cpp` at line 63, Replace the negation operator usage in the conditional with the coding-guideline-preferred form: change the if condition that currently reads using "!command_line_args.get_use_heuristic()" to use "false == command_line_args.get_use_heuristic()" instead; locate the check around the use of command_line_args.get_use_heuristic() in run.cpp and update the condition accordingly (preserve surrounding logic and braces).
♻️ Duplicate comments (3)
components/core/src/clp/ProfilerReporter.hpp (2)
25-30:⚠️ Potential issue | 🟡 MinorMinor documentation typos.
- Line 25: "destructured" should be "destructed", and "measurmenets" should be "measurements"
- Line 30: "accidentaly" should be "accidentally"
📝 Proposed fix
- * - Once the object is destructured (scope exit), the runtime measurmenets are reported. + * - Once the object is destructed (scope exit), the runtime measurements are reported.- * - Copy and move operations are removed to prevent accidentaly multiple reporting. + * - Copy and move operations are removed to prevent accidentally multiple reporting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp/ProfilerReporter.hpp` around lines 25 - 30, Fix three typos in the doc comment of ProfilerReporter: change "destructured" to "destructed", "measurmenets" to "measurements", and "accidentaly" to "accidentally" in the comment block describing the ProfilerReporter class (the paragraph that references ScopedProfiler and copy/move operations); no API changes, just update the text in the ProfilerReporter.hpp comment.
1-2: 🧹 Nitpick | 🔵 TrivialInclude guard naming inconsistency.
The include guard uses
CLP_PROFILER_REPORT_HPPbut the file is namedProfilerReporter.hpp. Consider usingCLP_PROFILER_REPORTER_HPPfor consistency.♻️ Proposed fix
-#ifndef CLP_PROFILER_REPORT_HPP -#define CLP_PROFILER_REPORT_HPP +#ifndef CLP_PROFILER_REPORTER_HPP +#define CLP_PROFILER_REPORTER_HPPAnd at line 65:
-#endif // CLP_PROFILER_REPORT_HPP +#endif // CLP_PROFILER_REPORTER_HPP🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp/ProfilerReporter.hpp` around lines 1 - 2, The include guard macro is inconsistent with the filename: replace the current macro CLP_PROFILER_REPORT_HPP with CLP_PROFILER_REPORTER_HPP in the `#ifndef` and `#define` lines and update the matching `#endif` comment (if present) to use CLP_PROFILER_REPORTER_HPP; ensure any duplicate occurrences of the old macro in this header are updated to the new CLP_PROFILER_REPORTER_HPP so the guard is consistent with ProfilerReporter.hpp.components/core/src/clp/ScopedProfiler.hpp (1)
47-48: 🧹 Nitpick | 🔵 TrivialConsider using
__COUNTER__for more robust unique identifiers.The macro uses
__LINE__which will cause name collisions ifPROFILE_SCOPEis used twice on the same line (e.g., in a single-lineifstatement or macro expansion). Using__COUNTER__(supported by GCC, Clang, and MSVC) would generate globally unique identifiers.♻️ Proposed enhancement
-#define PROFILE_SCOPE(x) \ - ::clp::ScopedProfiler CLP_CONCAT(__clp_profile_scope_, __LINE__)(x) +#define PROFILE_SCOPE(x) \ + ::clp::ScopedProfiler CLP_CONCAT(__clp_profile_scope_, __COUNTER__)(x)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp/ScopedProfiler.hpp` around lines 47 - 48, The macro PROFILE_SCOPE currently concatenates using __LINE__ which can collide when used multiple times on the same line; update the macro in ScopedProfiler.hpp to use __COUNTER__ instead of __LINE__ (e.g., change PROFILE_SCOPE definition that creates ::clp::ScopedProfiler CLP_CONCAT(__clp_profile_scope_, __LINE__)(x) to use __COUNTER__), and add a portable fallback that falls back to __LINE__ only if __COUNTER__ is not defined so CLP_CONCAT and PROFILE_SCOPE keep generating unique identifiers across expansions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/core/src/clp/clp/FileCompressor.cpp`:
- Around line 126-128: Remove the unused local variable file_name in
FileCompressor::compress_file: delete the line that assigns string file_name =
std::filesystem::canonical(file_to_compress.get_path()).string(); (and if that
was the only use of std::filesystem in this translation unit, consider also
removing the unused include or using directive). Ensure
PROFILE_SCOPE("FileCompressor::compress_file") remains and no other code
references file_name.
---
Outside diff comments:
In `@components/core/src/clp/clp/run.cpp`:
- Line 162: The condition uses negation operator on the boolean variable
compression_successful; change the check to use the coding-guideline form by
replacing the if (!compression_successful) test with if (false ==
compression_successful) (locate the occurrence in clp::run or the function
containing compression_successful in run.cpp and update that conditional).
- Line 63: Replace the negation operator usage in the conditional with the
coding-guideline-preferred form: change the if condition that currently reads
using "!command_line_args.get_use_heuristic()" to use "false ==
command_line_args.get_use_heuristic()" instead; locate the check around the use
of command_line_args.get_use_heuristic() in run.cpp and update the condition
accordingly (preserve surrounding logic and braces).
---
Duplicate comments:
In `@components/core/src/clp/ProfilerReporter.hpp`:
- Around line 25-30: Fix three typos in the doc comment of ProfilerReporter:
change "destructured" to "destructed", "measurmenets" to "measurements", and
"accidentaly" to "accidentally" in the comment block describing the
ProfilerReporter class (the paragraph that references ScopedProfiler and
copy/move operations); no API changes, just update the text in the
ProfilerReporter.hpp comment.
- Around line 1-2: The include guard macro is inconsistent with the filename:
replace the current macro CLP_PROFILER_REPORT_HPP with CLP_PROFILER_REPORTER_HPP
in the `#ifndef` and `#define` lines and update the matching `#endif` comment (if
present) to use CLP_PROFILER_REPORTER_HPP; ensure any duplicate occurrences of
the old macro in this header are updated to the new CLP_PROFILER_REPORTER_HPP so
the guard is consistent with ProfilerReporter.hpp.
In `@components/core/src/clp/ScopedProfiler.hpp`:
- Around line 47-48: The macro PROFILE_SCOPE currently concatenates using
__LINE__ which can collide when used multiple times on the same line; update the
macro in ScopedProfiler.hpp to use __COUNTER__ instead of __LINE__ (e.g., change
PROFILE_SCOPE definition that creates ::clp::ScopedProfiler
CLP_CONCAT(__clp_profile_scope_, __LINE__)(x) to use __COUNTER__), and add a
portable fallback that falls back to __LINE__ only if __COUNTER__ is not defined
so CLP_CONCAT and PROFILE_SCOPE keep generating unique identifiers across
expansions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b498685b-2eb1-45d8-9847-d6f2d497a0a3
📒 Files selected for processing (4)
components/core/src/clp/ProfilerReporter.hppcomponents/core/src/clp/ScopedProfiler.hppcomponents/core/src/clp/clp/FileCompressor.cppcomponents/core/src/clp/clp/run.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/core/src/clp/Profiler.hpp`:
- Around line 58-61: Update the class comment for Profiler to remove the blanket
"must be called before using the class" requirement and state that
Profiler::init() is only required for compile-time measurements; clarify that
the new runtime APIs work without calling Profiler::init(). Reference
Profiler::init() and the runtime APIs in the comment so callers know init is
optional except when using compile-time measurement paths.
- Around line 74-80: check_init() currently logs SPDLOG_ERROR every time
m_initialized is false (on every start/stop/get path); change it to log the
missing-init error only once by adding a one-time guard (e.g. a static
std::atomic<bool> or a static bool protected by std::once_flag) such as
m_init_error_logged and set it when you first detect !m_initialized, then call
SPDLOG_ERROR only when that flag was false and flip it to true; update the same
pattern in the other Profiler methods referenced (the start/stop/get code paths
at the 143-180 region) so all missing-init diagnostics are gated behind the
one-time flag while preserving the existing return of m_initialized and the
PROF_ACTIVE compile-time guard.
- Around line 95-113: The shared m_runtime_measurements unordered_map is not
synchronized: wrap all accesses (start_runtime_measurement,
stop_runtime_measurement, reset_runtime_measurements, check_runtime_timer_exists
and any iteration in ProfilerReporter) with a single mutex to prevent concurrent
container and Stopwatch races, and change get_runtime_measurements() to return a
copy/snapshot of the map (not a live reference) while holding the lock; ensure
PROFILE_SCOPE code paths also use these locked APIs so individual Stopwatch
start/stop calls are protected.
In `@components/core/src/clp/ProfilerReporter.hpp`:
- Around line 34-53: ProfilerReporter currently emits global runtime
measurements accumulated across prior reporter lifetimes; fix this by clearing
the global measurements at the start of the RAII window: in the ProfilerReporter
constructors (both ProfilerReporter() and explicit
ProfilerReporter(std::unordered_map<std::string, Stopwatch>& sink)) call a
reset/clear API on the Profiler (e.g., Profiler::clear_runtime_measurements() or
Profiler::reset_runtime_measurements()) so the profiler map starts empty for
this reporter instance; if such an API doesn't exist, add one that empties the
internal map returned by Profiler::get_runtime_measurements() and invoke it from
the ProfilerReporter constructors while keeping the destructor behavior that
writes into m_sink or logs as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c01dc71e-8df1-4ecc-891c-a700d86c805b
📒 Files selected for processing (4)
components/core/src/clp/Profiler.hppcomponents/core/src/clp/ProfilerReporter.hppcomponents/core/src/clp/Stopwatch.cppcomponents/core/src/clp/clp/FileCompressor.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR significantly simplifies profiling by introducing two new classes:
ScopedProfiler- Starts and stops a timer for a given name on construction/destruction.ProfilerReporter- Reports runtime measurements on destruction, including total time, call count, and average time.Usage:
ProfilerReporterinmain(before anyScopedProfilerinstances).PROFILE_SCOPE("scope_name")at the top of any method to track its runtime. This automatically creates aScopedProfiler.Changes to
Profilerclass:Compile-timemeasurements andRuntimemeasurements:Profiler::init().ScopedProfilerandProfilerReporter. Does not require callingProfiler::init().Other changes:
PROF_TEST_ENABLED=1in CMake to ensure profiling is always active during unit-tests.Stopwatchunit-tests to use modernsleepcalls and run faster.Validation Performed
ScopedProfiler.ProfilerReporter.Summary by CodeRabbit
New Features
Tests
Chores