feat: generate CPU & PGO profiles #6058
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request expands profiling and tracing support within the application. It updates the exclusion rules in the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as Application
participant Mem as Memory Profiler
participant CPU as CPU Profiler
participant Logger as Logger
User->>App: Start Application
App->>Mem: Initiate Memory Profiling (.mem file)
App->>CPU: Initiate CPU Profiling (.cpu file)
App->>Logger: Log profiling start and file creation
App->>App: Execute processing tasks
App->>CPU: Stop CPU Profiling
App->>Mem: Clean up Memory Profiling
App->>Logger: Log profiling completion/errors
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/nuclei/main.go (1)
108-156: LGTM! Well-structured profiling implementation.The changes improve the profiling implementation by:
- Using consistent file extensions (.mem, .cpu, .trace)
- Adding CPU profiling support
- Improving error messages with file names
- Proper cleanup in defer function
Consider consolidating the error handling for file creation to reduce code duplication:
- memProfileFile, err := os.Create(memProfile + ".mem") - if err != nil { - gologger.Fatal().Msgf("profile: could not create memory profile %q file: %v", memProfileFile.Name(), err) - } - - cpuProfileFile, err := os.Create(memProfile + ".cpu") - if err != nil { - gologger.Fatal().Msgf("profile: could not create CPU profile %q file: %v", cpuProfileFile.Name(), err) - } - - traceFile, err := os.Create(memProfile + ".trace") - if err != nil { - gologger.Fatal().Msgf("profile: could not create trace %q file: %v", traceFile.Name(), err) - } + createProfileFile := func(ext, profileType string) *os.File { + f, err := os.Create(memProfile + ext) + if err != nil { + gologger.Fatal().Msgf("profile: could not create %s profile %q file: %v", profileType, f.Name(), err) + } + return f + } + + memProfileFile := createProfileFile(".mem", "memory") + cpuProfileFile := createProfileFile(".cpu", "CPU") + traceFile := createProfileFile(".trace", "trace")DESIGN.md (1)
462-498: LGTM! Comprehensive profiling documentation.The documentation has been well-updated to include CPU profiling instructions and clear examples for analyzing both CPU and memory profiles.
Fix the bullet point formatting in the file list:
-This command creates three files: * `nuclei.cpu`: CPU profile * `nuclei.mem`: Memory (heap) profile * `nuclei.trace`: Execution trace +This command creates three files: + +* `nuclei.cpu`: CPU profile +* `nuclei.mem`: Memory (heap) profile +* `nuclei.trace`: Execution trace🧰 Tools
🪛 LanguageTool
[uncategorized] ~470-~470: Loose punctuation mark.
Context: ...and creates three files: *nuclei.cpu: CPU profile *nuclei.mem: Memory (hea...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/generate-pgo.yamlis excluded by!**/*.yaml.github/workflows/perf-test.yamlis excluded by!**/*.yaml.github/workflows/tests.yamlis excluded by!**/*.yaml.goreleaser.ymlis excluded by!**/*.yml
📒 Files selected for processing (4)
.gitignore(1 hunks)DESIGN.md(1 hunks)Makefile(2 hunks)cmd/nuclei/main.go(2 hunks)
🧰 Additional context used
🪛 LanguageTool
DESIGN.md
[uncategorized] ~470-~470: Loose punctuation mark.
Context: ...and creates three files: * nuclei.cpu: CPU profile * nuclei.mem: Memory (hea...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (5)
cmd/nuclei/main.go (1)
35-35: LGTM! Added PGO support.The addition of
-pgo=autoto GOFLAGS enables Profile-Guided Optimization, which can improve runtime performance by optimizing the code based on profiling data..gitignore (1)
48-53: LGTM! Added patterns for profiling files.The changes correctly add patterns to ignore the new profiling file extensions (*.mem, *.cpu) while maintaining the existing *.trace pattern.
Makefile (3)
13-15: Static Linking Flag Update
Appending-extldflags "-static"toLDFLAGS(instead of overwriting it) ensures that any pre-existing linker flags remain intact while enforcing static linking on non-Darwin systems. This approach aligns well with the profiling requirements to create consistent builds.
32-32: Enhanced Build Reproducibility with -trimpath
Introducing the-trimpathflag in the build command helps remove local file system paths from the compiled binary, thereby improving build reproducibility. This is a beneficial change given the focus on profiling and performance optimizations.
35-35: Incorporating PGO Support in GOFLAGS
The update ofGOFLAGSto include-pgo=autoensures that profile-guided optimization is applied during the build process. This change directly supports the PR’s objective of generating CPU and PGO profiles. Just verify that your build tooling and environment fully support this flag to avoid any unexpected behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/nuclei/main.go (2)
119-121: Consider using a struct to manage profile files.While the current implementation works, managing multiple profile files could be improved by using a struct to encapsulate the file handles and their cleanup logic.
+type profileFiles struct { + mem *os.File + cpu *os.File + trace *os.File +} + +func (p *profileFiles) cleanup() { + if p.mem != nil { + p.mem.Close() + } + if p.cpu != nil { + p.cpu.Close() + } + if p.trace != nil { + p.trace.Close() + } +}
136-152: Consider adding error handling for file close operations.The deferred cleanup function should handle potential errors when closing files.
defer func() { if err := pprof.WriteHeapProfile(memProfileFile); err != nil { gologger.Fatal().Msgf("profile: could not write memory profile: %v", err) } pprof.StopCPUProfile() - memProfileFile.Close() - traceFile.Close() + if err := memProfileFile.Close(); err != nil { + gologger.Warning().Msgf("profile: could not close memory profile: %v", err) + } + if err := traceFile.Close(); err != nil { + gologger.Warning().Msgf("profile: could not close trace file: %v", err) + } trace.Stop() runtime.MemProfileRate = oldMemProfileRate gologger.Info().Msgf("CPU profile saved at %q", cpuProfileFile.Name()) gologger.Info().Msgf("Memory usage snapshot saved at %q", memProfileFile.Name()) gologger.Info().Msgf("Traced at %q", traceFile.Name()) }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(2 hunks)cmd/nuclei/main.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (5)
cmd/nuclei/main.go (2)
111-117: LGTM! Well-structured helper function for profile file creation.The
createProfileFilehelper function effectively standardizes profile file creation with proper error handling and clear error messages.
131-134: LGTM! Proper CPU profiling setup.The CPU profiling setup is correctly implemented with proper error handling.
Makefile (3)
14-14: LGTM! Proper LDFLAGS appending.Using
+=instead of=correctly preserves existing linker flags while adding static linking.
29-30: Consider using environment variable for CGO setting.Setting CGO_ENABLED directly in the command might affect global state.
32-32:❓ Verification inconclusive
Verify PGO profile availability.
The
-pgo=autoflag requires a default.pgo profile to be present in the package directory for optimization.
🏁 Script executed:
#!/bin/bash # Check if default.pgo exists in the package directory find . -name "default.pgo" -type fLength of output: 34
PGO Profile Verification Required
It appears that no
default.pgofile was found in the repository (as confirmed by running the search command), yet the build target in the Makefile at line 32 uses the-pgo=autoflag which expects this file. Please manually verify that thedefault.pgofile is either intentionally omitted or should be generated/placed in the correct directory. If the absence of this file is unintended, either add it to enable profile-guided optimizations or remove the-pgo=autoflag to avoid potential build issues.
also adjust memory (heap) profiles ext to `.mem` Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
and append LDFLAGS ifneq "darwin" Signed-off-by: Dwi Siswanto <[email protected]>
* replace `go mod tidy` with `go mod download` and `go mod verify` * adjust indentations * add `-trimpath` flag * set `-pgo` flag to "`auto`" * add `ldflags` * quoting 386 GOARCH value Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
This reverts commit ee87720.
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
dwisiswant0
force-pushed
the
dwisiswant0/feat/generate-CPU-profiles
branch
from
6e048dd to
a7456e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cmd/nuclei/main_test.go (1)
16-87: LGTM! The benchmark test is well-structured.The benchmark test is well-implemented with proper setup, cleanup, and error handling. The test server and options configuration provide a good baseline for performance testing.
Consider these improvements:
- Add different payload sizes and response types to benchmark various scenarios.
- Add sub-benchmarks using
b.Run()to test different configurations:func BenchmarkRunEnumeration(b *testing.B) { testCases := []struct { name string responseStatus int bulkSize int templateThreads int }{ {"SmallPayload204", http.StatusNoContent, 25, 25}, {"LargePayload200", http.StatusOK, 50, 50}, // Add more cases... } for _, tc := range testCases { b.Run(tc.name, func(b *testing.B) { // Existing benchmark code with tc parameters }) } }DESIGN.md (4)
468-472: Improve Markdown List Styling
The list items that describe the files created (lines 468–472) currently use asterisks. According to markdownlint (MD004), using dashes for unordered lists is preferred for consistency. Consider updating the list items as follows:- * `nuclei.cpu`: CPU profile - * `nuclei.mem`: Memory (heap) profile - * `nuclei.trace`: Execution trace + - `nuclei.cpu`: CPU profile + - `nuclei.mem`: Memory (heap) profile + - `nuclei.trace`: Execution traceThis change will help maintain a consistent style in the document.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~470-~470: Loose punctuation mark.
Context: ...and creates three files: *nuclei.cpu: CPU profile *nuclei.mem: Memory (hea...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
470-470: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
471-471: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
472-472: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
476-480: Consistent List Formatting in Analysis Steps
In the "Analyzing the CPU/Memory Profiles" section (starting at line 476), the instruction items (e.g., “* View the profile in the terminal:”) also use asterisks. For consistency with the project’s markdown style guidelines, please switch these to dashes. Ensure that all similar list items (including those in subsequent analysis steps) follow the dash style.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
476-476: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
488-492: Uniform List Style for Additional Analysis Steps
Similar to the previous list styling comment, the list item for "Display top memory consumers:" (lines 488–492) should also use a dash instead of an asterisk for consistency.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
488-488: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
494-498: Standardize Web-Based Visualization List Formatting
The list item introducing the web-based visualization command (lines 494–498) uses an asterisk. For uniformity across the document, update this to a dash.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
494-494: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.github/workflows/generate-pgo.yamlis excluded by!**/*.yaml.github/workflows/perf-regression.yamlis excluded by!**/*.yaml.github/workflows/perf-test.yamlis excluded by!**/*.yaml.github/workflows/tests.yamlis excluded by!**/*.yaml.goreleaser.ymlis excluded by!**/*.yml
📒 Files selected for processing (6)
.gitignore(1 hunks)DESIGN.md(1 hunks)Makefile(2 hunks)cmd/nuclei/main.go(2 hunks)cmd/nuclei/main_test.go(1 hunks)pkg/output/output.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 LanguageTool
DESIGN.md
[uncategorized] ~470-~470: Loose punctuation mark.
Context: ...and creates three files: * nuclei.cpu: CPU profile * nuclei.mem: Memory (hea...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
DESIGN.md
470-470: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
471-471: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
472-472: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
476-476: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
482-482: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
484-484: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
488-488: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
494-494: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (12)
pkg/output/output.go (1)
75-75: LGTM! Clean implementation of stdout control.The addition of
DisableStdoutfield and its environment variable control is well-implemented. This feature is particularly useful for benchmarking and testing scenarios where stdout noise needs to be suppressed.Also applies to: 283-285
cmd/nuclei/main.go (4)
109-117: LGTM! Well-structured helper function for profile file creation.The
createProfileFilehelper function provides a clean abstraction for creating profile files with proper error handling.
119-121: LGTM! Consistent profile file creation.The profile files are created consistently using the helper function with appropriate extensions and types.
131-134: LGTM! Proper CPU profiling setup.CPU profiling is correctly initialized with proper error handling.
137-152: LGTM! Comprehensive cleanup and logging.The cleanup logic is thorough:
- Writes heap memory snapshot
- Stops CPU profiling
- Closes all profile files
- Provides clear logging of profile locations
Makefile (3)
14-14: LGTM! Improved LDFLAGS handling.Using
+=instead of=forLDFLAGSpreserves any existing flags while adding static linking for non-Darwin systems.
29-30: LGTM! Enhanced build flags.The addition of
-trimpathimproves build reproducibility by removing file system paths from the binary.
32-33: LGTM! Well-structured test build target.The new
build-testtarget is properly configured with:
- PGO auto flag for profile-guided optimization
- Separate output path for test binary
- CGO disabled for better portability
Also applies to: 37-44
DESIGN.md (4)
460-463: Clear Introduction to Profiling & Tracing
The updated introduction (lines 460–463) clearly explains that users can generate CPU and memory profiles plus an execution trace using the-profile-memflag. This enhancement aligns well with the PR objective of expanding profiling support.
482-486: Verification of Code Block Annotations
The code blocks used for the profiling commands already specify a language (using ```bash), which is good practice as it improves readability and syntax highlighting. No changes needed here.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
482-482: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
484-484: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
500-508: Comprehensive Trace File Analysis Section
The section detailing how to analyze the trace file (lines 500–508) is clear and complete. It effectively guides users to examine the execution trace using thego tool tracecommand, which supports the PR’s objective of enhanced profiling.
560-562: Overall Section Clarity & Completeness
The updated "Profiling and Tracing" section now comprehensively covers the process—from profile generation to analysis—with clear, step-by-step instructions and relevant code snippets. This aligns well with the PR’s goal of providing CPU and PGO profile support and adds valuable insight into performance diagnostics for the user.
Signed-off-by: Dwi Siswanto <[email protected]>
Proposed changes
profile-memFlag #6057DISABLE_STDOUTto Suppress Terminal Output #6060Proof of Concept
The following steps demonstrate the process of compiling and benchmarking the
nucleibinary with and w/o PGO.Steps
Compile the test binary and run the benchmark:
make build-test DISABLE_STDOUT=1 ./bin/nuclei.test -test.run - -test.bench=. -test.benchmem -test.count=10 ./cmd/nuclei/ | tee nopgo.txtGenerate CPU profiles:
Alternatively, extract this PGO file (pgo.zip) into the
cmd/nuclei/directory - Duration: 95.70s, Total samples = 45.96s (48.03%).Recompile with PGO and rerun the benchmark:
make build-test DISABLE_STDOUT=1 ./bin/nuclei.test -test.run - -test.bench=. -test.benchmem -test.count=10 ./cmd/nuclei/ | tee withpgo.txtBenchmark Results
This suggests that PGO can help optimize memory efficiency, although the improvements might not always be dramatic. The effectiveness of PGO largely depends on the quality of the CPU samples used, which can vary based on the workload and profiling data. However, I'm still exploring ways to gather more comprehensive CPU samples - potentially by adjusting
types.Optionsor expanding the number ofTargetsto improve sampling accuracy. The PGO data in this example was generated using the defaulttypes.Optionsfrom the CLI with only a single dummy server as the target, which may not fully capture real-world performance patterns.Checklist
Summary by CodeRabbit
Summary by CodeRabbit