add egg v4 #25
add egg v4 #25
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughConsolidates startup into a CLI-driven dispatcher (framework, port, workers, reusePort, startMode), adds Koa2 startup path and startKoa2(workers, port), updates run/test harness to launch multiple service variants with dedicated logs, changes package dependencies (egg4, koa2), and adds Node.js 25 to CI matrix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (simple/dispatch.js)
participant Primary as Primary Process
participant Worker as Worker Process
participant KoaMod as simple/koa2.js
participant Framework as framework.startCluster
CLI->>Primary: invoke with (framework, port, workers, startMode, reusePort)
alt framework == "koa2"
Primary->>Primary: if primary -> fork N workers
Primary->>Worker: worker process starts (inherits args)
Worker->>KoaMod: require('./koa2') and call startKoa2(port)
KoaMod-->>Worker: listen on port
else other framework
Primary->>Framework: require(framework).startCluster({ startMode, workers, port, reusePort, ... })
Framework-->>Primary: framework handles cluster lifecycle
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
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 |
Summary of ChangesHello @fengmk2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the latest beta version of the Egg.js framework, Egg.js v4, into the project's benchmarking suite. The changes involve updating dependencies, configuring the new framework for cluster-based execution, and modifying the benchmark script to include performance tests for Egg.js v4, while also adjusting the parameters for existing framework benchmarks to ensure consistency and potentially faster execution of the benchmark suite. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for benchmarking egg version 4. The changes include updating package.json with the new dependency, modifying simple/dispatch.js to start egg4 clusters, and updating simple/run.sh to include egg4 in the benchmark runs. I've found a critical compatibility issue with the usage of cluster.isPrimary which will break on older supported Node.js versions. My review also includes suggestions to improve code maintainability by reducing duplication and removing commented-out code, as well as a minor formatting fix in package.json.
simple/dispatch.js
Outdated
| } | ||
|
|
||
| if (cluster.isMaster) { | ||
| if (cluster.isPrimary) { |
There was a problem hiding this comment.
You've updated cluster.isMaster to cluster.isPrimary. While this is a good practice for modern Node.js versions, cluster.isPrimary was only introduced in Node.js v16.0.0. The package.json specifies "node": ">=14.20.0", which means this code will fail on Node.js v14 and v15. To ensure backward compatibility, you should check for both properties.
| if (cluster.isPrimary) { | |
| if (cluster.isPrimary || cluster.isMaster) { |
package.json
Outdated
| } | ||
| "license": "MIT", | ||
| "packageManager": "[email protected]" | ||
| } |
There was a problem hiding this comment.
The file is missing a newline character at the end. It's a common convention to end files with a newline to avoid issues with file concatenation and some command-line tools.
| } | |
| } | |
simple/dispatch.js
Outdated
| egg4.startCluster({ | ||
| workers, | ||
| baseDir: __dirname, | ||
| port: 7010, | ||
| framework: 'egg4', | ||
| }); | ||
|
|
||
| egg4.startCluster({ | ||
| workers, | ||
| baseDir: __dirname, | ||
| port: 7011, | ||
| framework: 'egg4', | ||
| reusePort: true, | ||
| }); |
There was a problem hiding this comment.
These two calls to egg4.startCluster are very similar and contain duplicated code. To improve maintainability, consider refactoring this into a loop to avoid repetition.
[
{ port: 7010 },
{ port: 7011, reusePort: true },
].forEach(config => {
egg4.startCluster({
workers,
baseDir: __dirname,
framework: 'egg4',
...config,
});
});| # sleep 5 | ||
| # echo "" | ||
| # echo "------- egg4 hello with reusePort=true -------" | ||
| # echo "" | ||
| # print_head "egg4" "egg4 hello with reusePort=true" | ||
| # wrk 'http://127.0.0.1:7011/' \ | ||
| # -d 10 \ | ||
| # -c 50 \ | ||
| # -t 4 \ | ||
| # --latency \ | ||
| # -s $REPORT |
There was a problem hiding this comment.
This block of code is commented out. If this benchmark is not ready or intended to be run, it should be removed to avoid cluttering the script. Dead code can be confusing for future maintainers.
There was a problem hiding this comment.
Pull request overview
This pull request adds support for egg v4 (beta) to the benchmark suite, updates the Node.js cluster API to use the modern isPrimary instead of the deprecated isMaster, and standardizes benchmark parameters across all tests.
- Adds egg4 framework with two server configurations (standard and with reusePort)
- Updates cluster API from deprecated
isMastertoisPrimary - Standardizes benchmark parameters: reduces test duration from 30s to 10s and thread count from 8 to 4 across all tests
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| package.json | Adds egg4 as npm:egg@beta dependency and specifies packageManager field |
| simple/dispatch.js | Imports egg4, adds two egg4 server configurations (ports 7010-7011), updates to cluster.isPrimary, removes egg-cluster version logging |
| simple/run.sh | Adds curl health checks and benchmark test for egg4, comments out egg4 reusePort benchmark, standardizes all wrk parameters to -d 10 -t 4 |
| .github/workflows/nodejs.yml | Adds Node.js version 25 to CI test matrix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
simple/dispatch.js
Outdated
| egg4.startCluster({ | ||
| workers, | ||
| baseDir: __dirname, | ||
| port: 7011, | ||
| framework: 'egg4', | ||
| reusePort: true, | ||
| }); | ||
|
|
There was a problem hiding this comment.
The egg4 server with reusePort on port 7011 is started in dispatch.js but the corresponding benchmark test is commented out. This creates an inconsistency where the server runs but is never tested. Either uncomment the benchmark test or remove the server configuration from dispatch.js.
| egg4.startCluster({ | |
| workers, | |
| baseDir: __dirname, | |
| port: 7011, | |
| framework: 'egg4', | |
| reusePort: true, | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
simple/run.sh (1)
81-103: Clarify commented-out egg4 reusePort test.The egg4 cluster with
reusePort: truestarts on port 7011 (in dispatch.js), and a curl check runs against it (line 21), but the actual wrk benchmark is commented out. This leaves unused resources running.Consider either:
- Enabling the test if egg4 reusePort is ready
- Removing the cluster startup and curl check if not needed yet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/nodejs.yml(1 hunks)package.json(2 hunks)simple/dispatch.js(3 hunks)simple/run.sh(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
simple/dispatch.js (1)
simple_view/dispatch.js (1)
cluster(4-4)
simple/run.sh (2)
simple_passport/run.sh (1)
print_head(11-17)simple_view/run.sh (1)
print_head(25-31)
⏰ 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). (6)
- GitHub Check: Node.js / Test (ubuntu-latest, 25)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Agent
🔇 Additional comments (7)
.github/workflows/nodejs.yml (1)
15-15: LGTM - Node.js 25 added to CI matrix.Adding Node.js 25 is appropriate for benchmarking against the latest runtime. Note that Node.js 25 is a current (odd-numbered) release and not an LTS version, so benchmark results may vary as it evolves.
package.json (2)
13-13: Beta dependency is appropriate for benchmarking.Using
npm:egg@betais suitable for a benchmark repository to test pre-release versions. Be aware that benchmark results may change as the beta evolves toward stable release.
39-39: [email protected] is a valid version available on the npm registry. ThepackageManagerfield pinning is appropriate.simple/dispatch.js (2)
62-75: LGTM - egg4 cluster configurations added.The egg4 clusters follow the established pattern from egg3:
- Port 7010: standard cluster configuration
- Port 7011: with
reusePort: truefor SO_REUSEPORT supportConfiguration is consistent with existing clusters.
13-13: Good modernization tocluster.isPrimary.Using
cluster.isPrimaryis the correct modern API.cluster.isMasterwas deprecated in Node.js 16.0.0.simple/run.sh (2)
19-21: LGTM - Health checks added for egg4 endpoints.Curl checks for ports 7010 and 7011 ensure the egg4 clusters are responding before benchmarks run.
39-41: Reduced test parameters across all benchmarks.Duration reduced from 30s to 10s and threads from 8 to 4. This speeds up CI but produces less stable benchmark results. If this is intentional for CI efficiency, consider documenting the trade-off or providing a separate "full benchmark" mode.
Also applies to: 51-53, 63-65, 75-77
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
simple/run.sh (1)
105-115: Commented-out code creates maintenance burden.This commented block for egg4 with reusePort should either be implemented or removed. Having it commented out makes it unclear whether it's intentionally disabled or forgotten.
🧹 Nitpick comments (1)
simple/dispatch.js (1)
23-35: Consider validating theframeworkargument.The
frameworkvariable is passed directly torequire(). While this is a benchmark tool and likely not exposed to untrusted input, validating against a known list would be safer and provide better error messages for typos.const VALID_FRAMEWORKS = ['koa2', 'egg1', 'egg2', 'egg3', 'egg4']; if (!VALID_FRAMEWORKS.includes(framework)) { throw new Error(`Invalid framework: ${framework}. Valid options: ${VALID_FRAMEWORKS.join(', ')}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.json(3 hunks)simple/dispatch.js(1 hunks)simple/koa2.js(1 hunks)simple/run.sh(3 hunks)simple/start_koa2.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (3)
simple/koa2.js (2)
simple_view/koa2.js (4)
Koa(1-1)router(4-4)app(6-6)n(7-7)simple/dispatch.js (2)
startKoa2(3-3)port(7-7)
simple/run.sh (4)
simple_passport/dispatch.js (1)
egg3(1-1)simple_view/dispatch.js (1)
egg3(1-1)simple_passport/run.sh (1)
print_head(11-17)simple_view/run.sh (1)
print_head(25-31)
simple/start_koa2.js (1)
simple/dispatch.js (3)
startKoa2(3-3)workers(11-11)port(7-7)
🪛 Shellcheck (0.11.0)
simple/run.sh
[warning] 13-13: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 18-18: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 23-23: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 28-28: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 33-33: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 153-153: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
⏰ 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). (5)
- GitHub Check: Node.js / Test (ubuntu-latest, 25)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
🔇 Additional comments (3)
simple/koa2.js (1)
4-23: LGTM!The function correctly wraps the Koa app setup and accepts a configurable port. The 15 pass-through middleware functions appear intentional for benchmarking middleware overhead.
simple/dispatch.js (1)
11-11:os.availableParallelism()requires Node.js 18.14.0+.This API was added in Node.js v18.14.0 and v19.4.0. For older Node.js versions, this will throw a TypeError. Consider adding a fallback.
-let workers = Number(process.argv[6] || os.availableParallelism()); +let workers = Number(process.argv[6] || (os.availableParallelism?.() ?? os.cpus().length));simple/run.sh (1)
38-45: LGTM!The curl health checks appropriately verify all started services are responding before running benchmarks.
| @@ -1,21 +1,23 @@ | |||
| const Koa = require('koa'); | |||
| const Koa = require('koa2'); | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
koa2 npm package vs koa difference
💡 Result:
Short answer
- "koa" is the official Koa framework package (use npm package koa; v2+ uses async/await). [2]
- "koa2" on npm is not the official core package — it’s an older/unofficial alpha/published package (and many community modules use "koa2" in their names to indicate Koa v2 compatibility). Do not use "koa2" as a substitute for the official framework; install "koa". [1][2]
Sources
- koa2 npm page (unofficial/alpha package). [1]
- koa (official package / v2 information). [2]
Change require('koa2') to require('koa').
The koa2 package on npm is an unofficial alpha package. The official Koa framework package is koa (v2+ supports async/await). Update line 1 to use the correct package name.
🤖 Prompt for AI Agents
In simple/koa2.js at line 1, change the incorrect import require('koa2') to
require('koa'); update any surrounding references if necessary, then ensure
package.json depends on "koa" (v2+), run npm install to update node_modules, and
verify the app starts with the official koa package.
simple/run.sh
Outdated
| EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js koa2 7001 2>&1 > $LOG/koa2.log & | ||
| koa2_pid=$! | ||
| echo "koa2 pid: $koa2_pid" |
There was a problem hiding this comment.
Fix stdout/stderr redirection order.
The current 2>&1 > file redirects stderr to the original stdout (terminal), then redirects stdout to file. To capture both stdout and stderr to the log file, the order must be reversed: > file 2>&1. This issue affects lines 13, 18, 23, 28, 33, and 153.
Apply this pattern to all affected lines:
-EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js koa2 7001 2>&1 > $LOG/koa2.log &
+EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js koa2 7001 > $LOG/koa2.log 2>&1 &Similarly for lines 18, 23, 28, 33:
-EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg3 7030 false 2>&1 > $LOG/egg3.log &
+EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg3 7030 false > $LOG/egg3.log 2>&1 &And line 153:
-kill $koa2_pid $egg3_pid $egg3_reusePort_pid $egg3_workerThreads_reusePort_pid $egg4_pid 2>&1 > $LOG/kill.log || true
+kill $koa2_pid $egg3_pid $egg3_reusePort_pid $egg3_workerThreads_reusePort_pid $egg4_pid > $LOG/kill.log 2>&1 || trueCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 13-13: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
🤖 Prompt for AI Agents
In simple/run.sh around lines 13, 18, 23, 28, 33 and 153, the stdout/stderr
redirection order is wrong (uses "2>&1 > file"), so change each occurrence to
redirect stdout first and then stderr to that file, e.g. replace "2>&1 >
$LOG/koa2.log" with "> $LOG/koa2.log 2>&1" (and apply the same "> <file> 2>&1"
pattern to the other five lines) so both stdout and stderr are captured in the
log file.
| const cluster = require('cluster'); | ||
|
|
||
| module.exports = function startKoa2(workers, port) { | ||
| if (cluster.isPrimary) { | ||
| for (let i = 0; i < workers; i++) { | ||
| cluster.fork(); | ||
| } | ||
| } else { | ||
| require('./koa2')(port); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f 'package.json' --exec cat {} \;Repository: eggjs/benchmark
Length of output: 1345
🏁 Script executed:
fd -t f 'start_koa2.js' --exec cat {} \;Repository: eggjs/benchmark
Length of output: 290
🏁 Script executed:
# Verify cluster API availability in Node.js v14
cat <<'EOF' > /tmp/check_cluster.js
// Check which cluster properties are available
// In Node.js v14, it should be cluster.isMaster
// In Node.js v16+, it should be cluster.isPrimary (and isMaster is deprecated)
console.log("cluster.isPrimary and cluster.isMaster availability info:");
console.log("Node.js v14.20.0: has cluster.isMaster (isPrimary not available)");
console.log("Node.js v16.0.0+: has cluster.isPrimary (isMaster deprecated)");
console.log("Node.js v18.0.0+: isMaster removed, only isPrimary available");
EOF
cat /tmp/check_cluster.jsRepository: eggjs/benchmark
Length of output: 545
Use cluster.isMaster fallback for Node.js v14 compatibility.
The project supports Node.js v14.20.0+, but cluster.isPrimary was introduced in v16.0.0. This code will fail on Node.js v14. Use cluster.isPrimary ?? cluster.isMaster to support both older and newer versions.
Additionally, there's no handling for worker crashes—workers that exit unexpectedly won't be replaced.
🤖 Prompt for AI Agents
In simple/start_koa2.js around lines 1 to 11, replace direct use of
cluster.isPrimary with a compatibility check using cluster.isPrimary ??
cluster.isMaster to support Node.js v14, and add a cluster 'exit' event handler
in the primary/master process to log when a worker dies and immediately fork a
replacement so workers are automatically respawned on unexpected exits.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
simple/run.sh (2)
105-115: Commented-out benchmark blocks: remove or gate behind a flag.
Same maintainability issue noted in prior review comments—these chunks are dead code as-is.Also applies to: 129-152
13-13: Fix stdout/stderr redirection order so logs actually capture stderr.
This is the same issue called out previously (SC2069).-EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js koa2 7001 2>&1 > $LOG/koa2.log & +EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" koa2 7001 > "$LOG/koa2.log" 2>&1 & -EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg3 7030 false 2>&1 > $LOG/egg3.log & +EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" egg3 7030 false > "$LOG/egg3.log" 2>&1 & -EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg3 7031 true 2>&1 > $LOG/egg3_reusePort.log & +EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" egg3 7031 true > "$LOG/egg3_reusePort.log" 2>&1 & -EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg3 7032 true worker_threads 2>&1 > $LOG/egg3_workerThreads_reusePort.log & +EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" egg3 7032 true worker_threads > "$LOG/egg3_workerThreads_reusePort.log" 2>&1 & -EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg4 7040 false 2>&1 > $LOG/egg4.log & +EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" egg4 7040 false > "$LOG/egg4.log" 2>&1 & -kill $koa2_pid $egg3_pid $egg3_reusePort_pid $egg3_workerThreads_reusePort_pid $egg4_pid 2>&1 > $LOG/kill.log || true +kill "$koa2_pid" "$egg3_pid" "$egg3_reusePort_pid" "$egg3_workerThreads_reusePort_pid" "$egg4_pid" > "$LOG/kill.log" 2>&1 || trueAlso applies to: 18-18, 23-23, 28-28, 33-33, 153-153
🧹 Nitpick comments (2)
simple/run.sh (2)
3-9: Harden path handling + quoting forDIR/CSV/REPORT/LOGand therm -rf.
Backticks + unquoted vars are fragile (spaces, globbing) and makerm -rfriskier than necessary.-DIR=`dirname $0` -CSV=$DIR/../stats.csv -REPORT=$DIR/../report.lua +DIR="$(cd -- "$(dirname -- "$0")" && pwd)" +CSV="$DIR/../stats.csv" +REPORT="$DIR/../report.lua" ... -LOG=$DIR/logs -rm -rf $LOG -mkdir -p $LOG +LOG="$DIR/logs" +rm -rf -- "$LOG" +mkdir -p -- "$LOG"
37-46: Replace fixedsleep 8with a simple readiness wait (reduces flakiness).
Right now a slow startup turns into misleading curl failures.-sleep 8 +for url in \ + 'http://127.0.0.1:7001/' \ + 'http://127.0.0.1:7030/' \ + 'http://127.0.0.1:7031/' \ + 'http://127.0.0.1:7032/' \ + 'http://127.0.0.1:7040/' +do + for _ in {1..40}; do + curl -fsS "$url" >/dev/null && break + sleep 0.2 + done +done # koa2 curl 'http://127.0.0.1:7001/'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/nodejs.yml(1 hunks)package.json(2 hunks)simple/run.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- .github/workflows/nodejs.yml
🧰 Additional context used
🧬 Code graph analysis (1)
simple/run.sh (4)
simple_passport/dispatch.js (1)
egg3(1-1)simple_view/dispatch.js (1)
egg3(1-1)simple_passport/run.sh (1)
print_head(11-17)simple_view/run.sh (1)
print_head(25-31)
🪛 Shellcheck (0.11.0)
simple/run.sh
[warning] 13-13: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 18-18: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 23-23: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 28-28: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 33-33: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 153-153: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
⏰ 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). (4)
- GitHub Check: Node.js / Test (ubuntu-latest, 25)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
| # egg4 | ||
| curl 'http://127.0.0.1:7040/' | ||
|
|
||
| test `tail -c 1 $CSV` && printf "\n" >> $CSV |
There was a problem hiding this comment.
Make the CSV newline append safe when the file doesn’t exist (and quote vars).
test \tail -c 1 $CSV`is brittle and will error if$CSV` is missing.
-test `tail -c 1 $CSV` && printf "\n" >> $CSV
+if [[ -f "$CSV" ]] && [[ -s "$CSV" ]] && [[ "$(tail -c 1 -- "$CSV")" != $'\n' ]]; then
+ printf "\n" >> "$CSV"
+fi📝 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.
| test `tail -c 1 $CSV` && printf "\n" >> $CSV | |
| if [[ -f "$CSV" ]] && [[ -s "$CSV" ]] && [[ "$(tail -c 1 -- "$CSV")" != $'\n' ]]; then | |
| printf "\n" >> "$CSV" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 47-47: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In simple/run.sh around line 47, replace the brittle backticked tail call with a
safe existence/size check and quoted variable usage: first test that "$CSV"
exists/is non-empty (e.g. using [ -s "$CSV" ] or [ -f "$CSV" ] as appropriate),
then run tail (with -- and "$CSV" quoted) or otherwise append a newline only
when the file exists; ensure all references to $CSV are quoted and the printf >>
operation appends to the quoted "$CSV".
- Add V8 CPU profile analysis report identifying hotspots - Add optimization recommendations for TEGG runtime - Key findings: - promiseInitHook consumes ~7% CPU (async hooks overhead) - KeyedLoadIC_Megamorphic ~5.7% (polymorphic property access) - TEGG lifecycle management ~2.5% (object create/destroy) - Add analysis scripts for profile processing - Update run.sh to enable NODE_ENV=production and --prof flag 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
| const globalLifecycleList = this.getLifecycleList(); | ||
| const objLifecycleList = this.getObjectLifecycleList(obj); | ||
| // Run both in parallel | ||
| await Promise.all([ |
There was a problem hiding this comment.
这一条优化建议有点效果，从 Requests/sec: 106159.62 可以提升到 Requests/sec: 111621.71
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
profiler-egg-v4/REPORT.md (1)
374-383: Fix the Appendix command:analyze-profile.jsonly reads the first file argument.
As written, the globbed invocation will ignore all but the first matched file (since the script usesconst filePath = args[0];). Recommend pointing users toaggregate-profiles.jsfor multi-file aggregation.Suggested doc tweak:
-# Generate markdown report -node profiler-egg-v4/analyze-profile.js isolate-*.log-processed.txt +# Generate aggregated markdown report (multiple workers) +node profiler-egg-v4/aggregate-profiles.js isolate-*.log-processed.txt + +# Generate a single-file markdown report +node profiler-egg-v4/analyze-profile.js isolate-0x...-v8-....log-processed.txt
♻️ Duplicate comments (3)
simple/run.sh (3)
57-92: Remove or gate the large commented-out benchmark blocks to reduce dead code.
They make it harder to see the active benchmark path; consider deleting them or turning them into opt-in flags (e.g., env var toggles).Also applies to: 105-152
3-9: Fix stdout/stderr redirection order so logs actually capture errors.
Right now2>&1 > filesends stderr to the original stdout (usually the terminal), not the log file. Use> file 2>&1(or{ ...; } >file 2>&1) on all affected commands, including thekillline.-DIR=`dirname $0` +DIR="$(dirname -- "$0")" CSV=$DIR/../stats.csv REPORT=$DIR/../report.lua @@ -NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js koa2 7001 2>&1 > $LOG/koa2.log & +NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" koa2 7001 > "$LOG/koa2.log" 2>&1 & @@ -NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg3 7030 false 2>&1 > $LOG/egg3.log & +NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" egg3 7030 false > "$LOG/egg3.log" 2>&1 & @@ -NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg3 7031 true 2>&1 > $LOG/egg3_reusePort.log & +NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" egg3 7031 true > "$LOG/egg3_reusePort.log" 2>&1 & @@ -NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg3 7032 true worker_threads 2>&1 > $LOG/egg3_workerThreads_reusePort.log & +NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" egg3 7032 true worker_threads > "$LOG/egg3_workerThreads_reusePort.log" 2>&1 & @@ -NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS $DIR/dispatch.js egg4 7040 false 2>&1 > $LOG/egg4.log & +NODE_ENV=production EGG_SERVER_ENV=prod node $NODE_FLAGS "$DIR/dispatch.js" egg4 7040 false > "$LOG/egg4.log" 2>&1 & @@ -kill $koa2_pid $egg3_pid $egg3_reusePort_pid $egg3_workerThreads_reusePort_pid $egg4_pid 2>&1 > $LOG/kill.log || true +kill "$koa2_pid" "$egg3_pid" "$egg3_reusePort_pid" "$egg3_workerThreads_reusePort_pid" "$egg4_pid" > "$LOG/kill.log" 2>&1 || trueAlso applies to: 13-14, 18-19, 23-24, 28-30, 33-35, 153-153
47-47: Make CSV newline append safe when the file doesn’t exist (and quote vars).
Current command errors if$CSVis missing/empty and is also subject to word-splitting.-test `tail -c 1 $CSV` && printf "\n" >> $CSV +if [[ -f "$CSV" ]] && [[ -s "$CSV" ]] && [[ "$(tail -c 1 -- "$CSV")" != $'\n' ]]; then + printf "\n" >> "$CSV" +fi
🧹 Nitpick comments (10)
simple/run.sh (2)
7-9: Quote paths / avoid unquotedrm -rftarget.
If$DIRever contains spaces (or is empty due to a bug),rm -rf $LOGbecomes risky; quote it.-LOG=$DIR/logs -rm -rf $LOG -mkdir -p $LOG +LOG="$DIR/logs" +rm -rf -- "$LOG" +mkdir -p -- "$LOG"
37-45: Make the readiness checks fail fast (or retry) instead of blindsleep 8.
As-is, slow startup will makecurlfail but the script keeps going, potentially producing partial/invalid benchmark output. Considercurl -fsS --retry ...or a small wait-until loop per port.profiler-egg-v4/OPTIMIZATIONS.md (4)
7-45: Call out latency tradeoff ofawait teggCtx.destroy(...)(correctness vs throughput).
Awaiting destroy fixes “fire-and-forget” pileups, but it also pushes destroy time onto the request’s critical path; worth explicitly stating when this is acceptable (or recommending a bounded queue).
273-320: Avoid${proto.id}string keys for Symbol caching; preferWeakMapkeyed byproto.
The proposal assumesproto.idexists/stable and risks unbounded growth. AWeakMap<proto, Map<objName, symbol>>avoids leaks and doesn’t require an id.
203-270: Proxy method-cache proposal: ensure cache key supportsSymbolproperties + “method changed” semantics.
Caching bound methods is good for allocations, but be explicit that ifobj[p]changes after first access, the cached bound function will be stale. If that’s acceptable, document it; otherwise invalidate onset.
1-3: Optional: tighten wording (“CPU-profile analysis”, etc.) to satisfy LanguageTool.
Purely editorial; only if you keep LanguageTool as a required check.profiler-egg-v4/aggregate-profiles.js (1)
98-116: CLI: include filePath in error output for faster diagnosis.
Right now you print onlyerr.message; wrapping errors (or adding context) will make it easier to spot which profile failed parsing.profiler-egg-v4/analyze-profile.js (3)
91-97: Make Top-N resilient: sort by ticks before slicing.
This avoids silently wrong “top” lists if the profiler output format/order changes.getTopFunctions(n = 20) { - return this.jsFunctions.slice(0, n); + return [...this.jsFunctions].sort((a, b) => b.ticks - a.ticks).slice(0, n); } getTopBuiltins(n = 20) { - return this.builtins.slice(0, n); + return [...this.builtins].sort((a, b) => b.ticks - a.ticks).slice(0, n); }
26-89: If header parsing fails, fail fast with a clearer error.
Right nowtickscan remain0and the script still produces output (or crashes later). Consider throwing after parse if ticks is missing, including the file name.Also applies to: 188-220
50-53: Either parse Bottom-up, or drop the unused field/section toggle to reduce confusion.
You detect[Bottom up (heavy) profile]:but don’t parse it; either implement it or remove the dead path/field for now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
profiler-egg-v4/profiles/isolate-0x82080c000-47318-v8-47318.logis excluded by!**/*.logprofiler-egg-v4/profiles/isolate-0xa6480c000-47317-v8-47317.logis excluded by!**/*.logprofiler-egg-v4/profiles/isolate-0xb6280c000-47319-v8-47319.logis excluded by!**/*.logprofiler-egg-v4/profiles/isolate-0xc7480c000-47316-v8-47316.logis excluded by!**/*.log
📒 Files selected for processing (6)
profiler-egg-v4/AGGREGATED.md(1 hunks)profiler-egg-v4/OPTIMIZATIONS.md(1 hunks)profiler-egg-v4/REPORT.md(1 hunks)profiler-egg-v4/aggregate-profiles.js(1 hunks)profiler-egg-v4/analyze-profile.js(1 hunks)simple/run.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
profiler-egg-v4/aggregate-profiles.js (1)
profiler-egg-v4/analyze-profile.js (6)
fs(13-13)path(14-14)filePath(196-196)content(200-200)analyzer(201-201)args(190-190)
simple/run.sh (2)
simple_passport/run.sh (1)
print_head(11-17)simple_view/run.sh (1)
print_head(25-31)
🪛 LanguageTool
profiler-egg-v4/OPTIMIZATIONS.md
[grammar] ~1-~1: Use a hyphen to join words.
Context: # Egg.js v4 / TEGG Specific Code Optimizations Based on CP...
(QB_NEW_EN_HYPHEN)
[grammar] ~348-~348: Ensure spelling is correct
Context: ... Even consecutive get/set operations re-lookup the object Optimized Code: ```java...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
profiler-egg-v4/REPORT.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
129-129: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
307-307: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
312-312: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
317-317: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
profiler-egg-v4/AGGREGATED.md
9-9: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
17-17: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
30-30: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
46-46: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 Shellcheck (0.11.0)
simple/run.sh
[warning] 13-13: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 18-18: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 23-23: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 28-28: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 33-33: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 153-153: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
⏰ 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). (4)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (ubuntu-latest, 25)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
| addProfile(filePath) { | ||
| const content = fs.readFileSync(filePath, 'utf-8'); | ||
| const analyzer = new ProfileAnalyzer(); | ||
| analyzer.parse(content); | ||
| this.analyzers.push({ filePath, analyzer }); | ||
|
|
||
| this.aggregated.totalTicks += analyzer.ticks; | ||
| this.aggregated.totalUnaccounted += analyzer.unaccounted; | ||
|
|
||
| for (const fn of analyzer.jsFunctions) { | ||
| const existing = this.aggregated.functions.get(fn.name) || { ticks: 0, count: 0 }; | ||
| existing.ticks += fn.ticks; | ||
| existing.count += 1; | ||
| this.aggregated.functions.set(fn.name, existing); | ||
| } | ||
|
|
||
| for (const fn of analyzer.builtins) { | ||
| const existing = this.aggregated.builtins.get(fn.name) || { ticks: 0, count: 0 }; | ||
| existing.ticks += fn.ticks; | ||
| existing.count += 1; | ||
| this.aggregated.builtins.set(fn.name, existing); | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against 0 ticks: current percent calculations can throw RangeError and crash the CLI.
Infinity.toFixed(...) throws. This is reachable if header parsing fails (ticks stays 0) or a profile is empty/partial.
Suggested fix:
class AggregatedAnalyzer {
constructor() {
@@
}
+ static _safePercent(numerator, denominator, digits = 2) {
+ if (!denominator || !Number.isFinite(denominator) || denominator <= 0) return (0).toFixed(digits);
+ const v = (numerator / denominator) * 100;
+ return Number.isFinite(v) ? v.toFixed(digits) : (0).toFixed(digits);
+ }
+
+ static _escapeMdCell(s) {
+ return String(s)
+ .replace(/\|/g, '\\|')
+ .replace(/\r?\n/g, '<br/>');
+ }
+
addProfile(filePath) {
const content = fs.readFileSync(filePath, 'utf-8');
const analyzer = new ProfileAnalyzer();
analyzer.parse(content);
+ if (!analyzer.ticks) {
+ throw new Error(`Failed to parse ticks from profile header: ${filePath}`);
+ }
this.analyzers.push({ filePath, analyzer });
@@
getSortedFunctions(map, n = 30) {
const entries = Array.from(map.entries())
.map(([name, data]) => ({
name,
ticks: data.ticks,
avgTicks: Math.round(data.ticks / data.count),
count: data.count,
- percent: ((data.ticks / this.aggregated.totalTicks) * 100).toFixed(2)
+ percent: AggregatedAnalyzer._safePercent(data.ticks, this.aggregated.totalTicks, 2)
}))
.sort((a, b) => b.ticks - a.ticks);
@@
generateReport() {
let report = `# Aggregated CPU Profile Analysis\n\n`;
report += `## Overview\n`;
report += `- Profiles Analyzed: ${this.analyzers.length}\n`;
report += `- Total Ticks: ${this.aggregated.totalTicks}\n`;
- report += `- Total Unaccounted: ${this.aggregated.totalUnaccounted} (${((this.aggregated.totalUnaccounted / this.aggregated.totalTicks) * 100).toFixed(1)}%)\n\n`;
+ report += `- Total Unaccounted: ${this.aggregated.totalUnaccounted} (${AggregatedAnalyzer._safePercent(this.aggregated.totalUnaccounted, this.aggregated.totalTicks, 1)}%)\n\n`;
report += `## Per-Worker Summary\n`;
+ report += `\n`;
report += `| Worker | Ticks | JS% | Unaccounted% |\n`;
report += `|--------|-------|-----|------------|\n`;
for (const { filePath, analyzer } of this.analyzers) {
const name = path.basename(filePath).replace('-processed.txt', '');
const jsPercent = analyzer.summary['JavaScript']?.percent || 0;
- const unaccPercent = ((analyzer.unaccounted / analyzer.ticks) * 100).toFixed(1);
+ const unaccPercent = AggregatedAnalyzer._safePercent(analyzer.unaccounted, analyzer.ticks, 1);
report += `| ${name} | ${analyzer.ticks} | ${jsPercent}% | ${unaccPercent}% |\n`;
}
report += '\n';
report += `## Top 25 Aggregated JavaScript Functions\n`;
+ report += `\n`;
report += `| Total Ticks | Avg/Worker | % Total | Function |\n`;
report += `|-------------|------------|---------|----------|\n`;
for (const fn of this.getSortedFunctions(this.aggregated.functions, 25)) {
- report += `| ${fn.ticks} | ${fn.avgTicks} | ${fn.percent}% | ${fn.name} |\n`;
+ report += `| ${fn.ticks} | ${fn.avgTicks} | ${fn.percent}% | ${AggregatedAnalyzer._escapeMdCell(fn.name)} |\n`;
}
report += '\n';
report += `## Top 20 Aggregated V8 Builtins\n`;
+ report += `\n`;
report += `| Total Ticks | Avg/Worker | % Total | Builtin |\n`;
report += `|-------------|------------|---------|----------|\n`;
for (const fn of this.getSortedFunctions(this.aggregated.builtins, 20)) {
- report += `| ${fn.ticks} | ${fn.avgTicks} | ${fn.percent}% | ${fn.name} |\n`;
+ report += `| ${fn.ticks} | ${fn.avgTicks} | ${fn.percent}% | ${AggregatedAnalyzer._escapeMdCell(fn.name)} |\n`;
}Also applies to: 47-95
🤖 Prompt for AI Agents
In profiler-egg-v4/aggregate-profiles.js around lines 23 to 45 (also applies to
47-95), the code aggregates ticks but later percent calculations can divide by
zero (when totalTicks or other denominators are 0) causing Infinity.toFixed to
throw; update the percent calculation sites to guard against zero denominators
by checking if denominator === 0 and returning 0 (or "0.00") percent in that
case, otherwise perform the division and toFixed as before; implement this check
wherever percentages are computed (total-based and per-function/per-builtin) to
prevent RangeError crashes.
| ## Per-Worker Summary | ||
| | Worker | Ticks | JS% | Unaccounted% | | ||
| |--------|-------|-----|------------| | ||
| | isolate-0x82080c000-47318-v8-47318.log | 9016 | 54.1% | 45.7% | | ||
| | isolate-0xa6480c000-47317-v8-47317.log | 9021 | 53.2% | 46.8% | | ||
| | isolate-0xb6280c000-47319-v8-47319.log | 9031 | 53.7% | 46.3% | | ||
| | isolate-0xc7480c000-47316-v8-47316.log | 9008 | 54.8% | 23.0% | | ||
|
|
There was a problem hiding this comment.
Address markdownlint MD058 (blank lines around tables) to keep CI/doc lint clean.
Add a blank line before/after each table block.
Also applies to: 16-44, 45-68
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
9-9: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
In profiler-egg-v4/AGGREGATED.md around lines 8 to 15, the Markdown table lacks
the required blank line before and after the table per markdownlint rule MD058;
add a blank line immediately above the table header and one blank line
immediately after the table block. Apply the same change to the other table
blocks referenced (lines ~16-44 and ~45-68) so each table is separated by a
blank line above and below.
Fix broken Markdown tables: escape | in function names (renders incorrectly today).
The regex entry (Line 30) contains a literal pipe (|), which splits the table into extra columns (MD056) and drops content. Also consider stripping/shortening absolute /Users/... paths for portability.
Proposed minimal fix in this file (escape the pipe in that row; optional: wrap in backticks):
-| 84 | 21 | 0.23% | RegExp: ^[\^_`a-zA-Z\-0-9!#$%&'*+.|~]+$ |
+| 84 | 21 | 0.23% | RegExp: ^[\^_`a-zA-Z\-0-9!#$%&'*+.\|~]+$ |Also applies to: 16-44, 45-68
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
9-9: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
In profiler-egg-v4/AGGREGATED.md around lines 8-15 (and also apply same changes
to ranges 16-44 and 45-68), a table row contains an unescaped pipe character in
a function/regex entry which breaks the Markdown table rendering; edit the
affected rows to escape any literal '|' as '\|' or wrap the whole function/regex
cell in backticks to prevent column splitting, and while here replace long
absolute /Users/... paths with shortened basenames or a placeholder (e.g.,
filename only or {REDACTED_PATH}) for portability.
| parse(content) { | ||
| const lines = content.split('\n'); | ||
| let section = null; | ||
|
|
||
| for (let i = 0; i < lines.length; i++) { | ||
| const line = lines[i].trim(); | ||
|
|
||
| // Parse header | ||
| const headerMatch = line.match(/Statistical profiling result.*\((\d+) ticks, (\d+) unaccounted/); | ||
| if (headerMatch) { | ||
| this.ticks = parseInt(headerMatch[1], 10); | ||
| this.unaccounted = parseInt(headerMatch[2], 10); | ||
| continue; | ||
| } | ||
|
|
||
| // Detect section | ||
| if (line.startsWith('[JavaScript]:')) { | ||
| section = 'javascript'; | ||
| continue; | ||
| } | ||
| if (line.startsWith('[Summary]:')) { | ||
| section = 'summary'; | ||
| continue; | ||
| } | ||
| if (line.startsWith('[Bottom up (heavy) profile]:')) { | ||
| section = 'bottomup'; | ||
| continue; | ||
| } | ||
| if (line.startsWith('[C++]:') || line.startsWith('[C++ entry points]:') || line.startsWith('[Shared libraries]:')) { | ||
| section = null; | ||
| continue; | ||
| } | ||
|
|
||
| // Parse JavaScript functions | ||
| if (section === 'javascript' && line.match(/^\d+/)) { | ||
| const match = line.match(/^(\d+)\s+([\d.]+)%\s+([\d.]+)%\s+(.+)$/); | ||
| if (match) { | ||
| const entry = { | ||
| ticks: parseInt(match[1], 10), | ||
| totalPercent: parseFloat(match[2]), | ||
| nonlibPercent: parseFloat(match[3]), | ||
| name: match[4].trim() | ||
| }; | ||
|
|
||
| if (entry.name.startsWith('JS:') || entry.name.startsWith('RegExp:')) { | ||
| this.jsFunctions.push(entry); | ||
| } else if (entry.name.startsWith('Builtin:')) { | ||
| this.builtins.push(entry); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Parse summary | ||
| if (section === 'summary' && line.match(/^\d+/)) { | ||
| const match = line.match(/^(\d+)\s+([\d.]+)%\s+[\d.]+%?\s+(.+)$/); | ||
| if (match) { | ||
| this.summary[match[3].trim()] = { | ||
| ticks: parseInt(match[1], 10), | ||
| percent: parseFloat(match[2]) | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent report crashes on ticks === 0 and escape Markdown table cells (| breaks tables).
Both ((this.unaccounted / this.ticks) * 100).toFixed(1) and raw fn.name in tables can fail on edge inputs.
Suggested fix:
class ProfileAnalyzer {
@@
generateReport() {
const categories = this.categorize();
let report = `# V8 CPU Profile Analysis Report\n\n`;
report += `## Overview\n`;
report += `- Total Ticks: ${this.ticks}\n`;
- report += `- Unaccounted: ${this.unaccounted} (${((this.unaccounted / this.ticks) * 100).toFixed(1)}%)\n\n`;
+ const unaccPct = this.ticks ? (((this.unaccounted / this.ticks) * 100).toFixed(1)) : '0.0';
+ report += `- Unaccounted: ${this.unaccounted} (${unaccPct}%)\n\n`;
@@
report += `## Top 15 JavaScript Functions\n`;
report += `| Ticks | Total% | Function |\n`;
report += `|-------|--------|----------|\n`;
for (const fn of this.getTopFunctions(15)) {
- report += `| ${fn.ticks} | ${fn.totalPercent}% | ${fn.name} |\n`;
+ const name = String(fn.name).replace(/\|/g, '\\|').replace(/\r?\n/g, '<br/>');
+ report += `| ${fn.ticks} | ${fn.totalPercent}% | ${name} |\n`;
}
@@
report += `## Top 15 V8 Builtins\n`;
@@
for (const fn of this.getTopBuiltins(15)) {
- report += `| ${fn.ticks} | ${fn.totalPercent}% | ${fn.name} |\n`;
+ const name = String(fn.name).replace(/\|/g, '\\|').replace(/\r?\n/g, '<br/>');
+ report += `| ${fn.ticks} | ${fn.totalPercent}% | ${name} |\n`;
}Also applies to: 140-185
🤖 Prompt for AI Agents
In profiler-egg-v4/analyze-profile.js around lines 26-89 (also apply same
pattern to lines ~140-185), the parser currently computes percentages using
(this.unaccounted / this.ticks) * 100 and outputs raw function names into
Markdown tables, which can crash or break tables when ticks === 0 or names
contain pipe characters; update the code to guard against division by zero by
checking if this.ticks is > 0 before dividing (fall back to 0 or '0.0' percent
or 'N/A' as appropriate) and ensure any function/name strings used in Markdown
table cells have pipe characters escaped (replace '|' with '\|' or otherwise
sanitize cell content) and similarly defend any other percent calculations
against NaN/zero ticks so the report never throws and Markdown tables remain
valid.
| ``` | ||
| Function | Avg Ticks | Avg % | ||
| ---------------------------------------------------|-----------|------- | ||
| promiseInitHook (node:internal/async_hooks:328) | ~620 | 6.8% | ||
| promiseBeforeHook (node:internal/async_hooks:346) | ~85 | 0.9% | ||
| promiseAfterHook (node:internal/async_hooks:353) | ~70 | 0.8% | ||
| ``` |
There was a problem hiding this comment.
Add fence languages to satisfy MD040 and improve copy/paste UX.
Use text for ASCII tables/diagrams, bash for shell, etc. (markdownlint MD040).
Also applies to: 48-55, 71-81, 92-100, 106-116, 129-254, 307-320
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In profiler-egg-v4/REPORT.md around lines 31-37 (and also for ranges 48-55,
71-81, 92-100, 106-116, 129-254, 307-320), the markdown code fences lack
language specifiers which triggers MD040 and hinders copy/paste; update each
fenced block to include an appropriate language tag (e.g., use text for ASCII
tables/diagrams, bash for shell snippets, json/yaml for structured data, etc.)
so every ``` block becomes ```text, ```bash, etc., matching the content.
| **Recommendations**: | ||
| - Consider using `--no-async-hooks-promise-hooks` flag in production if async context tracking isn't needed | ||
| - Reduce unnecessary Promise wrapping | ||
| - Use native callbacks where possible for performance-critical paths | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find package.json and CI configuration to determine supported Node versions
find . -maxdepth 3 -type f \( -name "package.json" -o -name ".github" -o -name ".gitlab-ci.yml" -o -name ".travis.yml" -o -name "*.yml" \) | head -20Repository: eggjs/benchmark
Length of output: 184
🏁 Script executed:
# Check the repo structure
git ls-files | grep -E "(package\.json|\.github|\.gitlab|\.travis|ci)" | head -20Repository: eggjs/benchmark
Length of output: 174
🌐 Web query:
Node.js --no-async-hooks-promise-hooks flag support and availability
💡 Result:
Short answer: There is no documented Node.js CLI flag named --no-async-hooks-promise-hooks in official Node or V8 docs or release notes — Node controls promise-hook behavior via the async_hooks / v8 promiseHooks APIs (and promise tracking is disabled by default until async_hooks/promiseHooks are installed). To disable promise tracking you simply avoid installing async_hooks/promiseHooks (or don’t create/enable an AsyncHook); Node’s docs explain that promise execution tracking is opt‑in via async_hooks.createHook / v8.promiseHooks. [1][2][3]
Sources
- Node.js async_hooks (explains promise tracking is not assigned by default and is enabled when hooks are installed). [1]
- Node.js v8 / promiseHooks docs (describes promiseHooks API added in Node and how it interacts with async_hooks). [2]
- Node.js API (v22/v24 async_hooks / v8 pages showing the same behavior across releases). [3]
If you want, I can search Node.js GitHub issues/PRs for any mention of that exact CLI flag name (in case it appeared in an experiment or fork).
🏁 Script executed:
# Check Node version requirements in package.json
cat -n ./package.json | head -50Repository: eggjs/benchmark
Length of output: 1501
🏁 Script executed:
# Check the CI configuration for supported Node versions
cat -n ./.github/workflows/nodejs.ymlRepository: eggjs/benchmark
Length of output: 516
🏁 Script executed:
# Find and read REPORT.md
find . -name "REPORT.md" -type fRepository: eggjs/benchmark
Length of output: 86
🏁 Script executed:
# Read REPORT.md to see the actual recommendations at the specified lines
cat -n ./profiler-egg-v4/REPORT.md | sed -n '35,50p'Repository: eggjs/benchmark
Length of output: 902
🏁 Script executed:
# Check all three locations mentioned
cat -n ./profiler-egg-v4/REPORT.md | sed -n '320,340p'Repository: eggjs/benchmark
Length of output: 749
🏁 Script executed:
# Check the third location
cat -n ./profiler-egg-v4/REPORT.md | sed -n '425,445p'Repository: eggjs/benchmark
Length of output: 41
🏁 Script executed:
# Get the third location at lines 430-439
cat -n ./profiler-egg-v4/REPORT.md | sed -n '425,445p'Repository: eggjs/benchmark
Length of output: 41
Remove the --no-async-hooks-promise-hooks flag recommendation—it does not exist in any Node.js version.
The flag referenced in the recommendations (lines 42, 329) is not a real Node.js CLI flag and does not appear in any official Node.js documentation or release notes. Promise hook behavior in Node.js is controlled through the async_hooks API (opt-in via async_hooks.createHook() or v8.promiseHooks), not through a command-line flag.
Replace with accurate guidance: to reduce promise-hook overhead, avoid installing async_hooks in performance-critical code paths, or use native callbacks/synchronous patterns where applicable.
🤖 Prompt for AI Agents
In profiler-egg-v4/REPORT.md around lines 41 to 45 (and referenced at line
~329), remove the invalid recommendation to use the non-existent Node.js CLI
flag `--no-async-hooks-promise-hooks`; instead update the recommendation to
explain that promise-hook behavior is controlled programmatically via the
async_hooks API (e.g., avoid installing async_hooks or v8.promiseHooks in hot
code paths) and suggest using native callbacks or sync patterns to reduce
promise-hook overhead in performance-critical paths.
Summary by CodeRabbit
Infrastructure Updates
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.