Adds profiler lib #145
Adds profiler lib #145
Conversation
WalkthroughAdds an optional profiling subsystem. Build gains a PROFILER toggle and profiler library. New headers, sources, and tests implement RDTSC-based timing, page-fault counting, repetition testing, and anchor-based profiling. Main conditionally initializes, records, and flushes profiling to profile.txt. .gitignore updated; README added for profiler. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant App as Application (main)
participant Prof as Profiler (macros/API)
participant Perf as Perf (page faults)
User->>App: Launch (built with -DPROFILER)
App->>Prof: PROFILER_SETUP()
activate Prof
Prof->>Perf: perf_setup()
deactivate Prof
Note over App,Prof: Normal application work
App->>Prof: TIME_*_BEGIN/END blocks (optional)
Prof-->>App: Record anchors, bytes, times
User-->>App: Exit
App->>Prof: PROFILER_STOP(fp)
activate Prof
Prof->>Prof: prof_print(fp)
Prof->>Perf: perf_close()
deactivate Prof
sequenceDiagram
autonumber
participant Dev as Developer
participant CMake as CMake
participant Build as Build System
Dev->>CMake: -DPROFILER=ON
CMake->>Build: add_subdirectory(lib/profiler)
CMake->>Build: target_compile_definitions(-DPROFILER)
CMake->>Build: target_link_libraries(profiler)
Build-->>Dev: Executable with profiler enabled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Nitpick comments (11)
lib/profiler/src/rep_return_data.c (1)
94-94: Misleading test label.The label "malloc_static_data" is misleading since it tests heap-allocated data, not static data. Consider renaming to "malloc_allocated_data" or "heap_allocated_data" for clarity.
Apply this diff:
- { "malloc_static_data", test_allocated_data }, + { "heap_allocated_data", test_allocated_data },lib/profiler/include/perf.h (1)
1-2: Include guard uses reserved identifier.The include guard
_PERF_H_starts with an underscore followed by a capital letter, which is reserved for the implementation according to the C standard (7.1.3). While this typically works in practice, it's better to follow the standard.Apply this diff:
-#ifndef _PERF_H_ -#define _PERF_H_ +#ifndef PERF_H_ +#define PERF_H_And update Line 14:
-#endif // _PERF_H_ +#endif // PERF_H_lib/profiler/src/perf.c (1)
11-11: Reserved identifier used for static variable.The identifier
_FDstarts with an underscore followed by a capital letter, which is reserved for the implementation. Consider renaming toperf_fdor similar.Apply this diff:
-static int _FD; +static int perf_fd;Then update all references throughout the file (Lines 34, 38, 48, 57, 65).
lib/profiler/include/macros.h (2)
1-2: Include guard uses reserved identifier.The include guard
__MACROS_H_starts with a double underscore, which is reserved for the implementation according to the C standard. Use a name that doesn't start with underscore or double underscore.Apply this diff:
-#ifndef __MACROS_H_ -#define __MACROS_H_ +#ifndef PROFILER_MACROS_H_ +#define PROFILER_MACROS_H_And update Line 46:
-#endif // __MACROS_H_ +#endif // PROFILER_MACROS_H_
9-14: Consider wrapping multi-statement macros.The
PROFILER_STOPmacro contains multiple statements without do-while(0) wrapping. While this works in most contexts, it can cause issues with if-else statements without braces.Apply this diff:
-#define PROFILER_STOP(fp) \ - prof_stop(); \ - prof_print(fp); +#define PROFILER_STOP(fp) \ + do { \ + prof_stop(); \ + prof_print(fp); \ + } while(0)lib/profiler/include/internal/profiler_c.h (1)
1-2: Include guard uses reserved identifier.The include guard
_STOP_CLOCK_H_starts with an underscore followed by a capital letter, which is reserved for the implementation. Consider using a non-reserved name.Apply this diff:
-#ifndef _STOP_CLOCK_H_ -#define _STOP_CLOCK_H_ +#ifndef PROFILER_C_H_ +#define PROFILER_C_H_And update Line 45:
-#endif // _STOP_CLOCK_H_ +#endif // PROFILER_C_H_lib/profiler/src/profiler.c (2)
27-33: Masking assumes ANCHOR_CAPACITY is a power of twohash_index uses (capacity - 1) mask. Add a static assert or switch to modulo to prevent silent misindexing if capacity changes.
- size_t index = (size_t)(hash & (ANCHOR_CAPACITY - 1)); + // Ensure ANCHOR_CAPACITY is power-of-two + _Static_assert((ANCHOR_CAPACITY & (ANCHOR_CAPACITY - 1)) == 0, "ANCHOR_CAPACITY must be power-of-two"); + size_t index = (size_t)(hash & (ANCHOR_CAPACITY - 1));
11-13: Use 64‑bit constants portably for FNV‑1aOn some platforms UL isn’t 64‑bit. Prefer UINT64_C for exact width.
-#define FNV_OFFSET 14695981039346656037UL -#define FNV_PRIME 1099511628211UL +#include <stdint.h> +#define FNV_OFFSET UINT64_C(14695981039346656037) +#define FNV_PRIME UINT64_C(1099511628211)lib/profiler/src/rep_read_test.c (1)
167-170: Minor: use alloc_type consistently in header printUse alloc_type variable for the ternary to avoid confusion.
- printf("\n--- %s%s%s ---\n", - print_allocation_type(alloc_type), params.alloc_type - ? " + " : "", test_func->label); + printf("\n--- %s%s%s ---\n", + print_allocation_type(alloc_type), + alloc_type ? " + " : "", + test_func->label);lib/profiler/src/repetition_tester.c (2)
6-9: Don’t locally define PROFILER hereThis TU shouldn’t unilaterally set build‑time feature flags. Remove the local #define to keep config centralized.
-#define PROFILER #include "rdtsc.h" #include "perf.h"
95-96: Carriage return may be undesirable for file outputsPrinting “\r” even for non‑TTYs can produce confusing files. Consider restricting to TTYs.
- fprintf(fp, " \r"); + if (isatty(fileno(fp))) fprintf(fp, " \r");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.gitignore(1 hunks)CMakeLists.txt(3 hunks)lib/profiler/CMakeLists.txt(1 hunks)lib/profiler/README.md(1 hunks)lib/profiler/include/internal/common.h(1 hunks)lib/profiler/include/internal/profiler_c.h(1 hunks)lib/profiler/include/macros.h(1 hunks)lib/profiler/include/perf.h(1 hunks)lib/profiler/include/profiler.h(1 hunks)lib/profiler/include/rdtsc.h(1 hunks)lib/profiler/include/repetition_tester.h(1 hunks)lib/profiler/src/calc_cpu_freq.c(1 hunks)lib/profiler/src/perf.c(1 hunks)lib/profiler/src/profiler.c(1 hunks)lib/profiler/src/rdtsc.c(1 hunks)lib/profiler/src/rep_read_test.c(1 hunks)lib/profiler/src/rep_return_data.c(1 hunks)lib/profiler/src/repetition_tester.c(1 hunks)lib/profiler/test/calc_cpu_freq.c(1 hunks)src/main.c(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
lib/profiler/include/rdtsc.h (1)
lib/profiler/src/rdtsc.c (4)
get_os_time_freq(18-21)read_os_timer(23-28)read_cpu_timer(30-33)estimate_cpu_freq(35-55)
lib/profiler/src/perf.c (1)
src/main.c (1)
close(1357-1423)
lib/profiler/test/calc_cpu_freq.c (1)
lib/profiler/src/rdtsc.c (3)
get_os_time_freq(18-21)read_cpu_timer(30-33)read_os_timer(23-28)
lib/profiler/src/rep_return_data.c (2)
lib/profiler/src/repetition_tester.c (5)
rept_is_testing(98-146)rept_begin(38-45)rept_end(47-54)rept_count_bytes(56-60)rept_setup(10-36)lib/profiler/src/rdtsc.c (1)
estimate_cpu_freq(35-55)
lib/profiler/include/internal/profiler_c.h (1)
lib/profiler/src/profiler.c (6)
prof_start(52-55)prof_stop(57-60)prof_add_anchor(69-85)prof_print(119-174)make_anchor_block(176-182)read_anchor_block(184-189)
lib/profiler/include/repetition_tester.h (1)
lib/profiler/src/repetition_tester.c (6)
rept_setup(10-36)rept_begin(38-45)rept_end(47-54)rept_count_bytes(56-60)rept_is_testing(98-146)rept_print_results(148-158)
lib/profiler/include/perf.h (1)
lib/profiler/src/perf.c (4)
perf_setup(23-44)perf_reset_page_fault_count(46-52)perf_read_page_fault_count(54-61)perf_close(63-68)
lib/profiler/src/repetition_tester.c (2)
lib/profiler/src/perf.c (4)
perf_setup(23-44)perf_reset_page_fault_count(46-52)perf_read_page_fault_count(54-61)perf_close(63-68)lib/profiler/src/rdtsc.c (1)
read_cpu_timer(30-33)
lib/profiler/src/calc_cpu_freq.c (3)
lib/profiler/src/rep_read_test.c (1)
main(125-181)lib/profiler/src/rep_return_data.c (1)
main(89-112)lib/profiler/src/rdtsc.c (3)
get_os_time_freq(18-21)read_cpu_timer(30-33)read_os_timer(23-28)
lib/profiler/src/rep_read_test.c (3)
lib/profiler/src/repetition_tester.c (5)
rept_is_testing(98-146)rept_begin(38-45)rept_end(47-54)rept_count_bytes(56-60)rept_setup(10-36)lib/profiler/src/rep_return_data.c (1)
main(89-112)lib/profiler/src/rdtsc.c (1)
estimate_cpu_freq(35-55)
lib/profiler/src/profiler.c (2)
src/hashtable.c (1)
hash(48-58)lib/profiler/src/rdtsc.c (2)
read_cpu_timer(30-33)estimate_cpu_freq(35-55)
🪛 markdownlint-cli2 (0.18.1)
lib/profiler/README.md
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Mac (Clang)
- GitHub Check: Mac (GCC)
- GitHub Check: Ubuntu (Clang)
- GitHub Check: Ubuntu (GCC)
- GitHub Check: Windows (MSVC)
- GitHub Check: Ubuntu (mingw)
🔇 Additional comments (13)
.gitignore (1)
20-20: LGTM!The addition of
/profile.txtto the ignore list is appropriate given that the profiler writes its output to this file.lib/profiler/include/internal/common.h (1)
1-14: LGTM!The ANSI color code definitions follow standard conventions and will enable colorized terminal output for the profiler.
lib/profiler/include/rdtsc.h (1)
1-20: LGTM!The header properly declares the RDTSC timing API with appropriate guards and includes.
CMakeLists.txt (3)
13-13: LGTM!The PROFILER option is correctly defined as a CMake cache variable, allowing users to enable profiling at build time.
54-56: LGTM!The profiler subdirectory is conditionally included only when PROFILER is enabled, following proper CMake patterns.
223-227: LGTM!The profiler integration correctly:
- Adds the PROFILER compile definition
- Links the profiler library
- Includes the profiler headers
lib/profiler/include/profiler.h (1)
1-22: LGTM!The header follows best practices:
- Proper include guards
- C++ compatibility with
extern "C"blocks- Conditional inclusion based on PROFILER macro
lib/profiler/src/calc_cpu_freq.c (1)
8-31: LGTM!The CPU frequency estimation logic is correct:
- Records start times for both OS and CPU timers
- Waits for 1 second based on OS timer
- Calculates CPU frequency from elapsed CPU cycles and OS time
- Properly handles division by zero check
src/main.c (2)
65-67: LGTM!The conditional inclusion of the profiler header is correctly guarded by the PROFILER macro.
1472-1474: LGTM!The profiler setup call is correctly placed early in main(), before argument processing, ensuring profiling captures the entire application lifecycle.
lib/profiler/CMakeLists.txt (1)
1-15: LGTM!The CMake configuration is well-structured. The library target properly separates public and private include directories, and the source file list is clear.
lib/profiler/src/rep_return_data.c (1)
78-87: Verify NULL handling in test_allocated_data.The
malloc_create_data()call on Line 82 could return NULL if malloc fails, but there's no NULL check before dereferencing infree(data). Whilefree(NULL)is safe, the test would report incorrect byte counts and continue running with undefined behavior.Consider adding a NULL check:
static void test_allocated_data(RepetitionTester *tester) { while (rept_is_testing(tester)) { rept_begin(tester); ReturnData *data = malloc_create_data(); + if (!data) { + rept_error(tester, "malloc failed"); + break; + } rept_end(tester); free(data);lib/profiler/include/internal/profiler_c.h (1)
18-22: Profiler is statically allocated – no stack overflow risk. The global instance_Profis declaredstaticinprofiler.c, so it isn’t placed on the stack.Likely an incorrect or invalid review comment.
| static void print_row(FILE *fp, const char *label, RepetitionValue val, uint64_t rate) | ||
| { | ||
| static const double gb = 1024.0 * 1024.0 * 1024.0; | ||
| uint64_t test_count = val.E[RepVal_test_count]; | ||
| double divisor = test_count ? (double) test_count : 1; | ||
|
|
||
| double E[RepVal_count]; | ||
| for (size_t i = 0; i < RepVal_count; ++i) { | ||
| E[i] = (double) val.E[i] / divisor; | ||
| } | ||
|
|
||
| uint64_t time = E[RepVal_cpu_time]; | ||
| double seconds = (double) time/rate; | ||
| double millis = seconds*1000.0; | ||
| double gb_per_second = E[RepVal_byte_count] / (gb * seconds); | ||
|
|
||
| if (isatty(fileno(fp))) { | ||
| fprintf(fp, C_WHITE "%s:" C_GREEN " %lu " C_YELLOW "(%fms) " C_CYAN "%fgb/s" C_RESET, | ||
| label, time, millis, gb_per_second); | ||
| } else { | ||
| fprintf(fp, "%s: %lu (%fms) %fgb/s", label, time, millis, gb_per_second); | ||
| } | ||
|
|
||
| if (E[RepVal_page_faults] > 0) { | ||
| if (isatty(fileno(fp))) { | ||
| fprintf(fp, C_RED " PF: %0.4f (%0.4fk/fault)" C_RESET, | ||
| E[RepVal_page_faults], E[RepVal_byte_count]/(E[RepVal_page_faults] * 1024.0)); | ||
| } else { | ||
| fprintf(fp, " PF: %.4f (%.4fk/fault)", | ||
| E[RepVal_page_faults], | ||
| E[RepVal_byte_count]/(E[RepVal_page_faults] * 1024.0)); | ||
| } | ||
| } | ||
| fprintf(fp, " \r"); | ||
| } |
There was a problem hiding this comment.
Guard against zero rate and zero seconds; fix types in print_row
- rate can be 0; seconds may become 0; guard divisions.
- Use a dedicated tick variable; E[…] is double, so casting makes intent clearer.
- uint64_t time = E[RepVal_cpu_time];
- double seconds = (double) time/rate;
+ uint64_t ticks = (uint64_t)E[RepVal_cpu_time];
+ if (rate == 0) rate = 1;
+ double seconds = rate ? ((double)ticks / (double)rate) : 0.0;
double millis = seconds*1000.0;
- double gb_per_second = E[RepVal_byte_count] / (gb * seconds);
+ double gb_per_second = seconds > 0.0 ? (E[RepVal_byte_count] / (gb * seconds)) : 0.0;
@@
- fprintf(fp, C_WHITE "%s:" C_GREEN " %lu " C_YELLOW "(%fms) " C_CYAN "%fgb/s" C_RESET,
- label, time, millis, gb_per_second);
+ fprintf(fp, C_WHITE "%s:" C_GREEN " %lu " C_YELLOW "(%fms) " C_CYAN "%fgb/s" C_RESET,
+ label, (unsigned long)ticks, millis, gb_per_second);
} else {
- fprintf(fp, "%s: %lu (%fms) %fgb/s", label, time, millis, gb_per_second);
+ fprintf(fp, "%s: %lu (%fms) %fgb/s", label, (unsigned long)ticks, millis, gb_per_second);
}| printf(" OS Seconds: %.4f\n", (f64) os_elapsed/ (f64) os_freq); | ||
| printf(" CPU Timer: %lu -> %lu = %lu\n", cpu_start, cpu_end, cpu_elapsed); | ||
| printf(" CPU Freq: %lu (guessed)\n", cpu_freq); | ||
| } |
There was a problem hiding this comment.
Missing return statement.
The main function should explicitly return a status code. While C99 allows implicit return of 0 from main, being explicit improves clarity.
Apply this diff:
printf(" CPU Freq: %lu (guessed)\n", cpu_freq);
+ return 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| printf(" CPU Freq: %lu (guessed)\n", cpu_freq); | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In lib/profiler/test/calc_cpu_freq.c around line 31, the main function currently
ends with a closing brace without an explicit return; add an explicit return
statement (return 0;) immediately before the final closing brace of main so the
function returns a success status code.
| #ifdef PROFILER | ||
| FILE *fp = fopen("profile.txt", "w"); | ||
| if (fp == NULL) { | ||
| PROFILER_STOP(stdout); | ||
| } else { | ||
| PROFILER_STOP(fp); | ||
| } | ||
| fclose(fp); | ||
| #endif |
There was a problem hiding this comment.
Fix potential NULL pointer dereference.
If fopen fails and returns NULL, the code calls fclose(fp) on Line 1421, which will crash. The fclose should only be called when fp is not NULL.
Apply this diff to fix the issue:
#ifdef PROFILER
FILE *fp = fopen("profile.txt", "w");
if (fp == NULL) {
PROFILER_STOP(stdout);
} else {
PROFILER_STOP(fp);
+ fclose(fp);
}
- fclose(fp);
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifdef PROFILER | |
| FILE *fp = fopen("profile.txt", "w"); | |
| if (fp == NULL) { | |
| PROFILER_STOP(stdout); | |
| } else { | |
| PROFILER_STOP(fp); | |
| } | |
| fclose(fp); | |
| #endif | |
| #ifdef PROFILER | |
| FILE *fp = fopen("profile.txt", "w"); | |
| if (fp == NULL) { | |
| PROFILER_STOP(stdout); | |
| } else { | |
| PROFILER_STOP(fp); | |
| fclose(fp); | |
| } | |
| #endif |
🤖 Prompt for AI Agents
In src/main.c around lines 1414 to 1422, the code calls fclose(fp)
unconditionally even when fopen failed and fp is NULL; change the logic so that
after fopen("profile.txt","w") you check if fp is NULL and call
PROFILER_STOP(stdout) and skip fclose, otherwise call PROFILER_STOP(fp) and then
fclose(fp); ensure fclose is only invoked when fp is non-NULL.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores