Add comments, add end of file newlines, fix php 8.5 compilation

carlos-granados · derickr · commit 0173970efadc · 2025-12-10T16:52:26.000Z
diff --git a/.github/benchmark-files/bench.php b/.github/benchmark-files/bench.php
@@ -1,4 +1,11 @@
 <?php
+
+/**
+ * This file has been copied from the php-src project. It is just a php process that runs some fairly standard
+ * algorithms and which serves as an artificial benchmark. We removed a couple of functions which were taking
+ * long to run and which made these benchmarks too slow
+ */
+
 if (function_exists("date_default_timezone_set")) {
     date_default_timezone_set("UTC");
 }
diff --git a/.github/benchmark-files/benchmark.ini b/.github/benchmark-files/benchmark.ini
@@ -1,11 +1,16 @@
+; Needed so that the php scripts won't time out while being run
 max_execution_time=0
+; We remove all error handling, printing and displaying so that it does not interfere with the tests
 error_reporting=0
 display_errors=off
 log_errors=off
+; We enable the opcache both for cli and cgi modes, disable the jit compiler and disable opcache optimization
+; (so that the case where Xdebug is not loaded matches the case where it is loaded as Xdebug disables this optimization)
+; We also set opcache to use a file cache so that it can be shared between the two runs that we do of the Symfony code
 opcache.enable=1
 opcache.enable_cli=1
 opcache.jit=disable
 opcache.validate_timestamps=0
 opcache.optimization_level=0
 opcache.file_cache=/tmp/opcache/
-opcache.file_cache_only=1
+opcache.file_cache_only=1
diff --git a/.github/benchmark-files/rector.php b/.github/benchmark-files/rector.php
@@ -12,4 +12,4 @@
     ->withPaths([
         __DIR__ . '/.github/benchmark-files/rector.php',
     ])
-;
+;
diff --git a/.github/benchmark-files/xdebug-benchmark.ini b/.github/benchmark-files/xdebug-benchmark.ini
@@ -1,3 +1,4 @@
 zend_extension=xdebug.so
 xdebug.start_with_request=no
-xdebug.max_nesting_level=2048
+; We need this because some of the functions in bench.php do deep recursion and the default nesting level is not enough
+xdebug.max_nesting_level=2048
diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml
@@ -1,25 +1,22 @@
 name: Benchmark Xdebug performance
 
 on:
+    # This workflow runs on three triggers: Weekly on schedule, manually or when pull requests are labeled
     schedule:
         # Every Sunday at 03:00 UTC
         - cron: '0 3 * * 0'
     workflow_dispatch: 
     pull_request:
-        types: [opened, edited, reopened, synchronize]
+        types: [ labeled ]
 
 jobs:
     run-benchmark:
+        # When triggered when a pull request is labeled we check the name of the label
         if: |
             github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' ||
             (
                 github.event_name == 'pull_request' &&
-                (
-                    startsWith(github.event.pull_request.title, 'perf:') ||
-                    startsWith(github.event.pull_request.title, 'Perf:') ||
-                    startsWith(github.event.pull_request.title, 'Performance:') ||
-                    startsWith(github.event.pull_request.title, 'performance:')
-                )
+                github.event.label.name == 'performance-check'
             )
         runs-on: ubuntu-latest
         strategy:
@@ -36,11 +33,13 @@ jobs:
                 with:
                     php-version: "${{ matrix.php }}"
                     coverage: none
+                    # We need to use a specific version of Composer because we are using a specific version of the Symfony Demo
+                    # which is not compatible with the latest versions of Composer
                     tools: composer:v2.2.21
 
-            # ASLR can cause a lot of noise due to missed sse opportunities for memcpy
-            # and other operations, so we disable it during benchmarking.
             -   name: Disable ASLR
+                # ASLR can cause a lot of noise due to missed sse opportunities for memcpy
+                # and other operations, so we disable it during benchmarking.
                 run: echo 0 | sudo tee /proc/sys/kernel/randomize_va_space
 
             -   name: Install valgrind
@@ -58,6 +57,10 @@ jobs:
 
             -   name: install Symfony demo
                 if: matrix.command == 'symfony'   
+                # For versions of PHP greater than 8.4 we need to run a different version of the Symfony Demo.
+                # The older versions suffer from the nulllable parameter deprecation and this generated a lot of deprecations
+                # in the code and this skewed the tests results a lot. And we cannot use the newer version for older versions
+                # of PHP as it is not compatible.
                 run: |
                     if [ "${{ matrix.php }}" \< "8.4" ]; then
                         version="v2.0.2"
@@ -67,6 +70,10 @@ jobs:
                     git clone -q --branch "$version" --depth 1 https://github.com/symfony/demo.git ./symfony-demo
                     cd symfony-demo
                     composer install 
+                    # We do not want our tests to be skewed by any error, warning, notice or deprecation thrown by the code,
+                    # so we set the error level to 0 on purpose. Unfortunately, Symfony overrides this setup and sets their own
+                    # error reporting level. We fix this by changing all the places where Symfony is setting the error reporting
+                    # level so that it continues to be set to 0
                     find . -type f -name "*.php" -exec sed -i.bak -E 's/error_reporting\(.*\);/error_reporting(0);/g' {} +
 
             -   name: install Rector
@@ -97,19 +104,30 @@ jobs:
                 if: matrix.command == 'symfony'   
                 run: |
                     cd symfony-demo
+                    # The Symfony demo is run using php-cgi to simulate a web server. Symfony needs that this env variable is set,
+                    # it would usually be set by the web server, we need to set it manually instead
                     export SCRIPT_FILENAME="$(realpath public/index.php)"
+                    # Symfony also needs this to be set to a non empty value
+                    export APP_SECRET=APP_SECRET
+                    # We run the web page twice so that the files have already been compiled into the opcache in order to remove
+                    # the compilation time from the equation. This also better reflects the normal situation in a web server
                     php-cgi public/index.php
                     valgrind --tool=callgrind --dump-instr=yes --callgrind-out-file=callgrind.out -- php-cgi public/index.php
                     mv callgrind.out ..
                     cd ..
 
             -   name: benchmark rector.php
-                if: matrix.command == 'rector'   
+                if: matrix.command == 'rector' 
+                # Rector needs to be run with the --xdebug flag so that it does not try to disable Xdebug
+                # and also with the --debug flag so that it does not try to run several threads in parallel
                 run: valgrind --tool=callgrind --dump-instr=yes --callgrind-out-file=callgrind.out -- php vendor/bin/rector --dry-run --xdebug --debug || true
 
             -   name: save matrix values
                 run: |
+                    # We read the number of executed instructions from the callgrind.out file and save it in an artifact
+                    # to be processed later
                     awk '/^summary:/ { print $2 }' callgrind.out > results/php-${{ matrix.php }}_cmd-${{ matrix.command }}_xdebug-${{ matrix.xdebug_mode }}.txt
+                    # We also save the matrix variables so that in a later step we can build a list of the executed options
                     echo "${{ matrix.php }},${{ matrix.command }},${{ matrix.xdebug_mode }}" > results/matrix-values-${{ matrix.php }}-${{ matrix.command }}-${{ matrix.xdebug_mode }}.txt
 
             -   name: Upload result and matrix info
@@ -119,6 +137,8 @@ jobs:
                     path: results/
 
     performance:
+        # After running all benchmarks for all commands, php versions and Xdebug modes we run a new job that builds
+        # a summary of all execution times
         needs: run-benchmark
         runs-on: ubuntu-latest
         steps:
@@ -128,6 +148,8 @@ jobs:
                     path: results
 
             -   name: Merge matrix values
+                # We copy all the saved result files into a single folder and build a file with a list
+                # of all matrix variables used
                 run: |
                     mkdir merged
                     find results -name '*.txt' -exec cp {} merged/ \;
@@ -136,13 +158,16 @@ jobs:
 
             -   name: Generate summary table
                 run: |
+                    # This is needed to be able to print the number of instructions using commas for separators
                     export LC_NUMERIC=en_US.UTF-8
                     echo "# 🕒 Performance Results" > summary.md 
 
+                    # We build lists of the different matrix options used in the previous jobs
                     commands=$(cut -d',' -f2 unique-matrix-values.txt | sort | uniq)
                     php_versions=$(cut -d',' -f1 unique-matrix-values.txt | sort | uniq)
                     xdebug_modes=$(cut -d',' -f3 unique-matrix-values.txt | sort | uniq)
 
+                    # And we loop over all these versions building a summary with tables of all the values
                     for command in $commands; do
                         echo "" >> summary.md
                         echo "## **Command:** \`$command\`" >> summary.md
@@ -153,6 +178,8 @@ jobs:
                             echo "| Xdebug | Instructions | Slowdown |" >> summary.md
                             echo "|--------|-------------:|---------:|" >> summary.md
 
+                            # To calculate the slowdown, we read a base value which is taken from the data
+                            # where Xdebug mode is "no" (it was not loaded)
                             base_file="merged/php-${php}_cmd-${command}_xdebug-no.txt"
                             base_value=$(cat "$base_file")
                             for xdebug in $xdebug_modes; do
@@ -164,6 +191,7 @@ jobs:
                                     else
                                         slowdown=$(awk -v v=$value -v b=$base_value 'BEGIN { printf "%.1f%%", ((v - b) * 100) / b }')
                                     fi
+                                    # The number of instructions is formatted with thousands separators
                                     formatted_value=$(printf "%'d" "$value")
                                     echo "| $xdebug | $formatted_value | $slowdown |" >> summary.md
                                 fi
@@ -172,9 +200,13 @@ jobs:
                     done
 
             -   name: print summary
+                # This generated summary file is copied into a special file which GitHub has defined
+                # in this special env variable and it will use it to print a summary for the workflow
                 run: cat summary.md >> $GITHUB_STEP_SUMMARY
 
             -   name: delete intermediary artifacts
+                # The intermediate files that we generated are not needed and can be deleted,
+                # helping us keep the summary page clean
                 uses: geekyeggo/delete-artifact@v5
                 with:
                     name: results-*                
