feat: Add perf task to generate perf data. #1134
Conversation
…ion for functionality required for perf task
WalkthroughThe changes introduce new variables and tasks in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Taskfile
participant RemoteServer
participant LocalFS
participant PerfTool
User->>Taskfile: Invoke perf task
Taskfile->>Taskfile: Run download-dataset
Taskfile->>LocalFS: Check if dataset file exists
alt Dataset missing
Taskfile->>RemoteServer: Download tarball
Taskfile->>LocalFS: Extract and move log file
end
Taskfile->>Taskfile: Build package with BUILD_WITH_SYMBOLS=true
Taskfile->>PerfTool: Run perf record on binary with dataset
PerfTool->>LocalFS: Generate perf.data
Taskfile->>LocalFS: Change permissions on perf.data
Taskfile->>PerfTool: Run perf report, output perf-report.txt
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfile.yaml(6 hunks)tools/yscope-dev-utils(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
tools/yscope-dev-utils (1)
Learnt from: Bill-hbrhbr
PR: #1126
File: .gitignore:5-5
Timestamp: 2025-07-25T21:29:48.947Z
Learning: In the CLP project, the .clang-format file is maintained in the yscope-dev-utils submodule and copied over to the main CLP repository, so it should be ignored in .gitignore to prevent accidental commits of the copied file and maintain the single source of truth in the submodule.
taskfile.yaml (2)
Learnt from: anlowee
PR: #925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those in components/core/tools/scripts/lib_install/, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.
🔇 Additional comments (1)
taskfile.yaml (1)
99-120: Clean propagation ofBUILD_WITH_SYMBOLSflag – looks good
The variable is surfaced topackageand forwarded tocore, ensuring symbol builds only when requested.
taskfile.yaml
Outdated
| download-dataset: | ||
| status: | ||
| - "test -f {{.G_DATASET_LOCATION}}" | ||
| cmds: | ||
| - >- | ||
| curl -o output.tar.gz | ||
| https://zenodo.org/records/10516387/files/cockroachdb.tar.gz?download=1 | ||
| - "tar -xvzf output.tar.gz" | ||
| - "rm output.tar.gz" | ||
| - >- | ||
| mkdir -p "$(dirname "{{.G_DATASET_LOCATION}}")" | ||
| - "mv cockroachdb/cockroach.node1.log {{.G_DATASET_LOCATION}}" | ||
| - "rmdir cockroachdb" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Download task lacks robustness & safety flags
curldoesn’t follow redirects or fail on HTTP errors.- No checksum is verified; corrupt or tampered tarballs will go unnoticed.
- Minor: using
-vextraction is chatty andrmshould tolerate a missing file.
- - >-
- curl -o output.tar.gz
- https://zenodo.org/records/10516387/files/cockroachdb.tar.gz?download=1
- - "tar -xvzf output.tar.gz"
- - "rm output.tar.gz"
+ - >-
+ curl --fail --location --show-error --silent
+ --output output.tar.gz
+ https://zenodo.org/records/10516387/files/cockroachdb.tar.gz?download=1
+ # TODO: verify checksum here if reproducibility matters
+ - "tar -xzf output.tar.gz"
+ - "rm -f output.tar.gz"Adding --fail --location --show-error --silent makes the download step safer and quieter; switching to -xzf avoids verbose logs; rm -f prevents an exit on missing file.
🤖 Prompt for AI Agents
In taskfile.yaml lines 609 to 622, improve the download-dataset task by adding
curl flags --fail, --location, --show-error, and --silent to handle redirects,
fail on HTTP errors, and reduce output noise. Change tar extraction from -xvzf
to -xzf to suppress verbose output. Replace rm output.tar.gz with rm -f
output.tar.gz to avoid errors if the file is missing. Additionally, add a
checksum verification step after download to ensure file integrity before
extraction.
| @@ -1 +1 @@ | |||
| Subproject commit 76aecbd6d0db6e2078005431946b5c2e716c4291 | |||
| Subproject commit b965f19f3e11c548f32bf7ab8e28ff38906be018 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm the new submodule SHA is valid and pushed to the remote repository
Since this file only updates the yscope-dev-utils submodule pointer, please double-check that commit b965f19f3e11c548f32bf7ab8e28ff38906be018 exists on the upstream repo and contains the expected profiling-related changes. A quick script to verify:
If the command prints the SHA, the pointer is valid; otherwise, the commit hasn’t been pushed or the URL is wrong.
🏁 Script executed:
#!/bin/bash
# Verify the submodule commit exists on the remote
set -euo pipefail
url=$(git config -f .gitmodules --get submodule.tools/yscope-dev-utils.url)
echo "Checking commit on $url"
git ls-remote "$url" b965f19f3e11c548f32bf7ab8e28ff38906be018Length of output: 393
Submodule SHA verification failed
The referenced commit b965f19f3e11c548f32bf7ab8e28ff38906be018 wasn’t found on the remote https://github.com/y-scope/yscope-dev-utils.git (no output from git ls-remote).
Please address the pointer:
- Ensure
b965f19f3e11c548f32bf7ab8e28ff38906be018is pushed to the upstream repo. - If the SHA is incorrect, update the submodule reference in
.gitmodules(tools/yscope-dev-utils) to a valid commit. - After pushing or correcting, run
git submodule sync(and optionallygit submodule update --init --recursive) to confirm the pointer resolves.
🤖 Prompt for AI Agents
In tools/yscope-dev-utils at line 1, the submodule SHA reference
b965f19f3e11c548f32bf7ab8e28ff38906be018 is invalid or missing from the remote
repository. Verify that this commit exists and is pushed to the upstream repo.
If it does not exist, update the submodule reference in the .gitmodules file to
point to a valid commit SHA. After correcting the SHA or pushing the commit, run
git submodule sync and optionally git submodule update --init --recursive to
ensure the submodule pointer resolves correctly.
There was a problem hiding this comment.
This is wrong: y-scope/yscope-dev-utils@b965f19 is present.
There was a problem hiding this comment.
🧩 Analysis chain
@PacificViking, let me re-verify the submodule commit to understand what happened with my previous check.
🏁 Script executed:
#!/bin/bash
# Re-verify the submodule commit exists on the remote
set -euo pipefail
echo "=== Checking submodule configuration ==="
git config -f .gitmodules --get submodule.tools/yscope-dev-utils.url
echo -e "\n=== Attempting to verify commit on remote ==="
url=$(git config -f .gitmodules --get submodule.tools/yscope-dev-utils.url)
git ls-remote "$url" b965f19f3e11c548f32bf7ab8e28ff38906be018
echo -e "\n=== Checking if we can fetch the specific commit ==="
# Try a different approach - check if we can resolve the commit
git ls-remote "$url" | grep "b965f19f3e11c548f32bf7ab8e28ff38906be018" || echo "Commit not found in ls-remote output"
echo -e "\n=== Checking recent commits on main branch ==="
git ls-remote "$url" refs/heads/main
echo -e "\n=== Testing URL accessibility ==="
curl -s -I "https://github.com/y-scope/yscope-dev-utils/commit/b965f19f3e11c548f32bf7ab8e28ff38906be018" | head -1
Length of output: 1359
@PacificViking, you're absolutely right, and I apologize for the incorrect comment. My verification confirms that commit b965f19f3e11c548f32bf7ab8e28ff38906be018 does indeed exist and is actually the current HEAD of the main branch in the yscope-dev-utils repository.
The submodule reference is completely valid. My initial verification must have failed due to a temporary network issue or timing problem. Thank you for the correction and for providing the GitHub link as evidence.
The submodule update in this PR is proper and points to a valid commit.
🐰💙
(expressing gratitude for the correction)
🧠 Learnings used
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1126
File: .gitignore:5-5
Timestamp: 2025-07-25T21:29:48.947Z
Learning: In the CLP project, the .clang-format file is maintained in the yscope-dev-utils submodule and copied over to the main CLP repository, so it should be ignored in .gitignore to prevent accidental commits of the copied file and maintain the single source of truth in the submodule.
Learnt from: junhaoliao
PR: y-scope/clp#1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
Learnt from: junhaoliao
PR: y-scope/clp#988
File: components/log-viewer-webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsx:34-37
Timestamp: 2025-06-09T17:48:56.024Z
Learning: In the y-scope/clp project, prefer `false == <expression>` rather than `!<expression>` for boolean expressions in TypeScript/JavaScript files, as specified in the coding guidelines.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
taskfile.yaml (1)
222-249:.taskdirectory may not exist – first run will explodeBoth
rm -f .task/build_with_symbols_*.stampandtouch .task/build_with_symbols_{{.BUILD_WITH_SYMBOLS}}.stampassume the.taskdirectory is already present.
If it isn’t (e.g. on a clean checkout),touchfails and the whole pipeline aborts.- - "rm -f .task/build_with_symbols_*.stamp" - - "touch .task/build_with_symbols_{{.BUILD_WITH_SYMBOLS}}.stamp" + - "mkdir -p .task" + - "rm -f .task/build_with_symbols_*.stamp" + - "touch .task/build_with_symbols_{{.BUILD_WITH_SYMBOLS}}.stamp"
♻️ Duplicate comments (1)
taskfile.yaml (1)
633-639: Quote paths & variables in theperfcommand for safetyGuard against spaces (or future glob expansion) in paths:
- perf record -F 99 -g -- ./build/core/clp-s c --timestamp-key "timestamp" \ - --target-encoded-size 268435456 build/clp_cockroachdb_perf_output "{{.G_DATASET_LOCATION}}" + perf record -F 99 -g -- ./build/core/clp-s c --timestamp-key "timestamp" \ + --target-encoded-size 268435456 "build/clp_cockroachdb_perf_output" "{{.G_DATASET_LOCATION}}"A tiny nit, but it keeps the task robust if someone runs it from a path with spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfile.yaml(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
taskfile.yaml (10)
Learnt from: anlowee
PR: #925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
Learnt from: haiqi96
PR: #620
File: components/core/src/clp/clo/clo.cpp:198-198
Timestamp: 2024-12-02T22:36:17.847Z
Learning: The existing references to file_split_id in the codebase are acceptable and do not need to be changed.
Learnt from: jackluo923
PR: #1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in install-prebuilt-packages.sh, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like --strip-components=1 to handle potential tarball layout changes.
Learnt from: jackluo923
PR: #1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate set -eu and set -o pipefail commands rather than combining them into set -euo pipefail, to ensure uniform script structure across all platform build scripts.
Learnt from: jackluo923
PR: #1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: jackluo923
PR: #1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: jackluo923
PR: #1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like apt-get update && apt-get install or apk update && apk add), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: #1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like apt-get update && apt-get install or apk update && apk add), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: #1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like apt-get update && apt-get install or apk update && apk add), preserve this pattern for uniform script structure.
Learnt from: kirkrodrigues
PR: #881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those in components/core/tools/scripts/lib_install/, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.
🔇 Additional comments (1)
taskfile.yaml (1)
55-55:-fflag on the stamp-file cleanup is spot onAdding the force flag prevents the task from failing on a fresh clone when no stamp files exist yet.
| download-dataset: | ||
| status: | ||
| - "test -f {{.G_DATASET_LOCATION}}" | ||
| cmds: | ||
| - >- | ||
| curl --fail --location --show-error --silent | ||
| --output output.tar.gz | ||
| https://zenodo.org/records/10516387/files/cockroachdb.tar.gz?download=1 | ||
| - "tar -xzf output.tar.gz" | ||
| - "rm -f output.tar.gz" | ||
| - >- | ||
| mkdir -p "$(dirname "{{.G_DATASET_LOCATION}}")" | ||
| - "mv cockroachdb/cockroach.node1.log {{.G_DATASET_LOCATION}}" | ||
| - "rmdir cockroachdb" | ||
|
|
There was a problem hiding this comment.
rmdir cockroachdb will error ‑- tarball contains multiple files
The CockroachDB archive ships several .log files.
After moving only cockroach.node1.log, the directory is not empty, so rmdir returns exit status 1 and the task fails.
- - "mv cockroachdb/cockroach.node1.log {{.G_DATASET_LOCATION}}"
- - "rmdir cockroachdb"
+ - "mv cockroachdb/cockroach.node1.log {{.G_DATASET_LOCATION}}"
+ - "rm -rf cockroachdb"Optional but recommended:
- add a checksum to detect a corrupted download
- quote
{{.G_DATASET_LOCATION}}in thetest -fstatus check for path safety
📝 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.
| download-dataset: | |
| status: | |
| - "test -f {{.G_DATASET_LOCATION}}" | |
| cmds: | |
| - >- | |
| curl --fail --location --show-error --silent | |
| --output output.tar.gz | |
| https://zenodo.org/records/10516387/files/cockroachdb.tar.gz?download=1 | |
| - "tar -xzf output.tar.gz" | |
| - "rm -f output.tar.gz" | |
| - >- | |
| mkdir -p "$(dirname "{{.G_DATASET_LOCATION}}")" | |
| - "mv cockroachdb/cockroach.node1.log {{.G_DATASET_LOCATION}}" | |
| - "rmdir cockroachdb" | |
| download-dataset: | |
| status: | |
| - "test -f {{.G_DATASET_LOCATION}}" | |
| cmds: | |
| - >- | |
| curl --fail --location --show-error --silent | |
| --output output.tar.gz | |
| https://zenodo.org/records/10516387/files/cockroachdb.tar.gz?download=1 | |
| - "tar -xzf output.tar.gz" | |
| - "rm -f output.tar.gz" | |
| - >- | |
| mkdir -p "$(dirname \"{{.G_DATASET_LOCATION}}\")" | |
| - "mv cockroachdb/cockroach.node1.log {{.G_DATASET_LOCATION}}" | |
| - "rm -rf cockroachdb" |
🤖 Prompt for AI Agents
In taskfile.yaml lines 610 to 624, the command `rmdir cockroachdb` fails because
the tarball contains multiple files and the directory is not empty after moving
only one log file. Replace `rmdir cockroachdb` with a command that forcefully
removes the entire directory and its contents, such as `rm -rf cockroachdb`.
Additionally, quote `{{.G_DATASET_LOCATION}}` in the `test -f` status check to
handle paths safely, and optionally add a checksum verification step after
downloading to ensure file integrity.
Description
This will allow
task perfto download the cockroachdb dataset (https://zenodo.org/records/10516387) and run perf on it, outputting the perf.data file.Checklist
breaking change.
Validation performed
task perfworks as intended and contains symbols. Linting checks pass.task coreand other ways of directly building the core package produce a core package that doesn't contain debug symbols.Summary by CodeRabbit