Skip to content

Commit 2faa18b

Browse files
adamzieladamziel
authored
Preserve child wrapper identity in the native AST facade (#391)
Stacked on #381. #381's review surfaced a real semantics regression in the lazy native AST facade: every accessor on `WP_MySQL_Native_Parser_Node` calls into Rust and returns a freshly constructed PHP wrapper, so `get_first_child_node()` returns a different object every time. `WP_Parser_Node` has always given callers stable child identity — attach state to a child once, walk past it, walk back, the state is still there — and the lazy native facade is meant to keep that contract intact. This restores it. A per-AST `WP_MySQL_Native_AST_Cache` is created lazily on the root and shared by reference with every wrapper that gets interned through it. Each accessor looks the returned wrapper's `native_node_index` up in the cache and either returns the canonical instance or registers the new one. `materialize_native_children()` pulls children through the same cache so any mutation a caller made through `get_first_child_node()` before the parent went through `append_child()` survives into `$this->children` — same instance, same mutations. Tokens are unchanged. The public token API has no mutators and no callers in this repo rely on `WP_MySQL_Token` identity; if that becomes a need we can extend the cache. ## What's in here - `class-wp-mysql-native-ast-cache.php` — small holder, one per AST. - Native node accessors run results through `intern()` / `intern_all()` / `intern_nodes()`. - `materialize_native_children()` reuses the interned wrappers so prior mutations don't get lost. - Regression test covering same-instance reads, descendant/child identity sharing, and the mutate-then-materialize edge case. Skips when the native extension isn't loaded. - `run-native-ast-walk-benchmark.php` parses the MySQL server suite, walks each AST, and reports `parsed`, `walked_nodes`, `duration`, `peak_mem`, and an identity-stability flag. - `Native AST Walk Perf` workflow runs the benchmark on this PR and on the PR base (`codex/native-lazy-ast-facade`) on the same runner, so the identity-cache cost is measured apples-to-apples on every push. ## Performance Benchmarked on CI against the no-cache baseline, same runner, same corpus (69,567 queries, 4.8M walked nodes). Full numbers in PR comments — links below. | scenario | baseline (no cache) | this PR | delta | |---|---|---|---| | native parse only | 1.28s | 1.28s | 0% | | native walk duration | 3.52s | 3.33s | **+5% (+0.19s)** | | native walk qps | 19,766 | 20,884 | **−5%** | | native walk peak memory | 50.0MB | 60.5MB | **+10.5MB (+21%)** | | native walk identity stable | **FALSE** | true | regression fixed | Hot-path optimization in `intern_all()` (cache reference hoisted out of the loop, per-item logic inlined) plus a typed `intern_nodes()` fast path for accessors that return only nodes brought the time penalty down from an initial +17% to +5%. Memory delta is structural — one retained PHP wrapper per visited node — and is the price of stable identity. Native walk is still ~5× faster than the pure-PHP path while now preserving `WP_Parser_Node` semantics. - First measurement: #391 (comment) - After hot-path optimization: #391 (comment) ## Test plan - [x] PHPUnit suite passes on the pure-PHP path. - [x] PHPUnit suite passes with the native extension loaded; the new identity tests run instead of skipping. - [x] `Native AST Walk Perf` workflow reports the cache cost vs. the PR base on every push.
1 parent 5275791 commit 2faa18b

11 files changed

Lines changed: 862 additions & 37 deletions

‎.github/workflows/mysql-parser-extension-tests.yml‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,10 @@ jobs:
8181
exit( 1 );
8282
}
8383
'
84-
./vendor/bin/phpunit -c ./phpunit.xml.dist tests/mysql/WP_MySQL_Lexer_Tests.php tests/parser/WP_Parser_Node_Tests.php
84+
working-directory: packages/mysql-on-sqlite
85+
86+
- name: Run PHPUnit tests with parser extension
87+
run: php -d extension="$GITHUB_WORKSPACE/packages/php-ext-wp-mysql-parser/target/debug/libwp_mysql_parser.so" ./vendor/bin/phpunit -c ./phpunit.xml.dist
8588
working-directory: packages/mysql-on-sqlite
8689

8790
sqlite-driver-extension-tests:
@@ -149,3 +152,7 @@ jobs:
149152
exit( 1 );
150153
}
151154
'
155+
156+
- name: Run PHPUnit tests with SQLite driver using parser extension
157+
run: php -d extension="$GITHUB_WORKSPACE/packages/php-ext-wp-mysql-parser/target/debug/libwp_mysql_parser.so" ./vendor/bin/phpunit -c ./phpunit.xml.dist
158+
working-directory: packages/mysql-on-sqlite

‎.github/workflows/native-ast-perf.yml‎

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
name: Native AST Walk Perf
2+
3+
on:
4+
push:
5+
paths:
6+
- '.github/workflows/native-ast-perf.yml'
7+
- 'packages/mysql-on-sqlite/src/mysql/native/**'
8+
- 'packages/mysql-on-sqlite/tests/tools/run-native-ast-walk-benchmark.php'
9+
- 'packages/php-ext-wp-mysql-parser/**'
10+
pull_request:
11+
paths:
12+
- '.github/workflows/native-ast-perf.yml'
13+
- 'packages/mysql-on-sqlite/src/mysql/native/**'
14+
- 'packages/mysql-on-sqlite/tests/tools/run-native-ast-walk-benchmark.php'
15+
- 'packages/php-ext-wp-mysql-parser/**'
16+
workflow_dispatch:
17+
18+
concurrency:
19+
group: ${{ github.workflow }}-${{ github.ref }}
20+
cancel-in-progress: true
21+
22+
jobs:
23+
perf:
24+
name: Native AST walk benchmark
25+
runs-on: ubuntu-latest
26+
timeout-minutes: 25
27+
28+
steps:
29+
- name: Checkout repository
30+
uses: actions/checkout@v4
31+
32+
- name: Set up PHP
33+
uses: shivammathur/setup-php@v2
34+
with:
35+
php-version: '8.2'
36+
coverage: none
37+
38+
- name: Set up Rust
39+
uses: dtolnay/rust-toolchain@stable
40+
41+
- name: Install native build dependencies
42+
run: |
43+
sudo apt-get update
44+
sudo apt-get install -y libclang-dev
45+
echo "PHP_CONFIG=$(command -v php-config)" >> "$GITHUB_ENV"
46+
LIBCLANG_SO="$(find /usr/lib -name 'libclang.so*' | head -n 1)"
47+
echo "LIBCLANG_PATH=$(dirname "$LIBCLANG_SO")" >> "$GITHUB_ENV"
48+
49+
- name: Install Composer dependencies (root)
50+
uses: ramsey/composer-install@v3
51+
with:
52+
ignore-cache: "yes"
53+
composer-options: "--optimize-autoloader"
54+
55+
- name: Install Composer dependencies (mysql-on-sqlite)
56+
uses: ramsey/composer-install@v3
57+
with:
58+
working-directory: packages/mysql-on-sqlite
59+
ignore-cache: "yes"
60+
composer-options: "--optimize-autoloader"
61+
62+
- name: Download MySQL test query corpus
63+
working-directory: packages/mysql-on-sqlite
64+
run: bash tests/tools/mysql-download-tests.sh || true
65+
66+
- name: Build parser extension (release)
67+
run: cargo build --release
68+
working-directory: packages/php-ext-wp-mysql-parser
69+
70+
- name: Locate built extension
71+
run: |
72+
EXT="$GITHUB_WORKSPACE/packages/php-ext-wp-mysql-parser/target/release/libwp_mysql_parser.so"
73+
test -f "$EXT" || { echo "Extension not built at $EXT"; exit 1; }
74+
echo "NATIVE_EXT=$EXT" >> "$GITHUB_ENV"
75+
76+
- name: Benchmark — pure-PHP path (parse only)
77+
working-directory: packages/mysql-on-sqlite
78+
run: |
79+
php tests/tools/run-native-ast-walk-benchmark.php --no-walk | tee php-parse-only.txt
80+
81+
- name: Benchmark — pure-PHP path (walk)
82+
working-directory: packages/mysql-on-sqlite
83+
run: |
84+
php tests/tools/run-native-ast-walk-benchmark.php | tee php-walk.txt
85+
86+
- name: Benchmark — native path (parse only)
87+
working-directory: packages/mysql-on-sqlite
88+
run: |
89+
php -d extension="$NATIVE_EXT" tests/tools/run-native-ast-walk-benchmark.php --no-walk | tee native-parse-only.txt
90+
91+
- name: Benchmark — native path (walk, with identity cache)
92+
working-directory: packages/mysql-on-sqlite
93+
run: |
94+
php -d extension="$NATIVE_EXT" tests/tools/run-native-ast-walk-benchmark.php | tee native-walk.txt
95+
96+
- name: Check out baseline (PR base, no identity cache)
97+
run: |
98+
git fetch --no-tags --depth=1 origin codex/native-lazy-ast-facade
99+
git worktree add ../baseline FETCH_HEAD
100+
101+
- name: Install Composer dependencies (baseline mysql-on-sqlite)
102+
uses: ramsey/composer-install@v3
103+
with:
104+
working-directory: ../baseline/packages/mysql-on-sqlite
105+
ignore-cache: "yes"
106+
composer-options: "--optimize-autoloader"
107+
108+
- name: Build baseline parser extension (release)
109+
run: cargo build --release
110+
working-directory: ../baseline/packages/php-ext-wp-mysql-parser
111+
112+
- name: Stage benchmark + corpus into baseline
113+
run: |
114+
mkdir -p ../baseline/packages/mysql-on-sqlite/tests/mysql/data
115+
cp packages/mysql-on-sqlite/tests/tools/run-native-ast-walk-benchmark.php \
116+
../baseline/packages/mysql-on-sqlite/tests/tools/
117+
cp packages/mysql-on-sqlite/tests/mysql/data/*.csv \
118+
../baseline/packages/mysql-on-sqlite/tests/mysql/data/ 2>/dev/null || true
119+
120+
- name: Benchmark — baseline native path (walk, no identity cache)
121+
working-directory: ../baseline/packages/mysql-on-sqlite
122+
run: |
123+
BASE_EXT="$(realpath ../../packages/php-ext-wp-mysql-parser/target/release/libwp_mysql_parser.so)"
124+
php -d extension="$BASE_EXT" tests/tools/run-native-ast-walk-benchmark.php \
125+
| tee "$GITHUB_WORKSPACE/packages/mysql-on-sqlite/baseline-native-walk.txt"
126+
127+
- name: Benchmark — baseline native path (parse only)
128+
working-directory: ../baseline/packages/mysql-on-sqlite
129+
run: |
130+
BASE_EXT="$(realpath ../../packages/php-ext-wp-mysql-parser/target/release/libwp_mysql_parser.so)"
131+
php -d extension="$BASE_EXT" tests/tools/run-native-ast-walk-benchmark.php --no-walk \
132+
| tee "$GITHUB_WORKSPACE/packages/mysql-on-sqlite/baseline-native-parse-only.txt"
133+
134+
# Hit-heavy scenarios — these are where the per-AST identity cache is
135+
# supposed to win. The baseline reallocates wrappers on every accessor
136+
# call, while the PR reuses them. Run on both to make the gap visible.
137+
- name: Benchmark — native rewalk x10 (this PR)
138+
working-directory: packages/mysql-on-sqlite
139+
run: |
140+
php -d extension="$NATIVE_EXT" tests/tools/run-native-ast-walk-benchmark.php --mode=rewalk --repeat=10 \
141+
| tee native-rewalk.txt
142+
143+
- name: Benchmark — baseline rewalk x10
144+
working-directory: ../baseline/packages/mysql-on-sqlite
145+
run: |
146+
BASE_EXT="$(realpath ../../packages/php-ext-wp-mysql-parser/target/release/libwp_mysql_parser.so)"
147+
php -d extension="$BASE_EXT" tests/tools/run-native-ast-walk-benchmark.php --mode=rewalk --repeat=10 \
148+
| tee "$GITHUB_WORKSPACE/packages/mysql-on-sqlite/baseline-native-rewalk.txt"
149+
150+
- name: Benchmark — native reread x20 (this PR)
151+
working-directory: packages/mysql-on-sqlite
152+
run: |
153+
php -d extension="$NATIVE_EXT" tests/tools/run-native-ast-walk-benchmark.php --mode=reread --repeat=20 \
154+
| tee native-reread.txt
155+
156+
- name: Benchmark — baseline reread x20
157+
working-directory: ../baseline/packages/mysql-on-sqlite
158+
run: |
159+
BASE_EXT="$(realpath ../../packages/php-ext-wp-mysql-parser/target/release/libwp_mysql_parser.so)"
160+
php -d extension="$BASE_EXT" tests/tools/run-native-ast-walk-benchmark.php --mode=reread --repeat=20 \
161+
| tee "$GITHUB_WORKSPACE/packages/mysql-on-sqlite/baseline-native-reread.txt"
162+
163+
- name: Benchmark — native subtree x5 (this PR)
164+
working-directory: packages/mysql-on-sqlite
165+
run: |
166+
php -d extension="$NATIVE_EXT" tests/tools/run-native-ast-walk-benchmark.php --mode=subtree --repeat=5 \
167+
| tee native-subtree.txt
168+
169+
- name: Benchmark — baseline subtree x5
170+
working-directory: ../baseline/packages/mysql-on-sqlite
171+
run: |
172+
BASE_EXT="$(realpath ../../packages/php-ext-wp-mysql-parser/target/release/libwp_mysql_parser.so)"
173+
php -d extension="$BASE_EXT" tests/tools/run-native-ast-walk-benchmark.php --mode=subtree --repeat=5 \
174+
| tee "$GITHUB_WORKSPACE/packages/mysql-on-sqlite/baseline-native-subtree.txt"
175+
176+
- name: Summarize
177+
if: always()
178+
working-directory: packages/mysql-on-sqlite
179+
run: |
180+
extract() {
181+
# Pull a numeric field (e.g. duration=1.23s) from a benchmark
182+
# output line. Returns "n/a" if missing.
183+
local file="$1" key="$2"
184+
[ -f "$file" ] || { echo "n/a"; return; }
185+
grep -oE "${key}=[^ ]+" "$file" | head -1 | cut -d= -f2 || echo "n/a"
186+
}
187+
188+
{
189+
echo '### Native AST walk perf'
190+
echo
191+
echo '| scenario | result |'
192+
echo '|---|---|'
193+
for f in php-parse-only.txt php-walk.txt native-parse-only.txt native-walk.txt baseline-native-parse-only.txt baseline-native-walk.txt native-rewalk.txt baseline-native-rewalk.txt native-reread.txt baseline-native-reread.txt native-subtree.txt baseline-native-subtree.txt; do
194+
[ -f "$f" ] || continue
195+
line="$(cat "$f")"
196+
echo "| ${f%.txt} | \`$line\` |"
197+
done
198+
echo
199+
echo '### Identity-cache cost (native walk: with cache vs PR base without)'
200+
echo
201+
echo '| metric | with cache (this PR) | baseline | delta |'
202+
echo '|---|---|---|---|'
203+
for key in duration qps peak_mem walked_nodes; do
204+
with="$(extract native-walk.txt "$key")"
205+
base="$(extract baseline-native-walk.txt "$key")"
206+
echo "| $key | $with | $base | — |"
207+
done
208+
} >> "$GITHUB_STEP_SUMMARY"
209+
210+
- name: Upload raw output
211+
if: always()
212+
uses: actions/upload-artifact@v4
213+
with:
214+
name: native-ast-perf-${{ github.run_id }}
215+
path: packages/mysql-on-sqlite/*.txt
216+
if-no-files-found: warn

‎.github/workflows/wp-tests-phpunit.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,8 @@ jobs:
5454
- name: Build and load parser extension in WordPress PHP containers
5555
run: bash .github/workflows/wp-tests-phpunit-native-extension-setup.sh
5656

57-
- name: Verify WordPress uses parser extension
58-
run: cd wordpress && node tools/local-env/scripts/docker.js run --rm php php /var/www/native-verify-extension.php
57+
- name: Run WordPress PHPUnit tests with parser extension
58+
run: node .github/workflows/wp-tests-phpunit-run.js
5959

6060
- name: Stop Docker containers
6161
if: always()

‎packages/mysql-on-sqlite/src/load.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
if ( class_exists( 'WP_MySQL_Native_Parser', false ) ) {
2929
require_once __DIR__ . '/mysql/native/mysql-rust-bridge.php';
30+
require_once __DIR__ . '/mysql/native/class-wp-mysql-native-ast-cache.php';
3031
require_once __DIR__ . '/mysql/native/class-wp-mysql-native-parser-node.php';
3132
require_once __DIR__ . '/mysql/native/class-wp-mysql-parser.php';
3233
} else {

‎packages/mysql-on-sqlite/src/mysql/class-wp-mysql-parser.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,17 @@ class WP_MySQL_Parser extends WP_Parser {
88
*/
99
private $current_ast;
1010

11+
/**
12+
* Reset this parser with a new token stream.
13+
*
14+
* @param array<WP_Parser_Token> $tokens The parser tokens.
15+
*/
16+
public function reset_tokens( array $tokens ): void {
17+
$this->tokens = $tokens;
18+
$this->position = 0;
19+
$this->current_ast = null;
20+
}
21+
1122
/**
1223
* Parse the next query from the input SQL string.
1324
*

‎packages/mysql-on-sqlite/src/mysql/native/class-wp-mysql-native-ast-cache.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
3+
/**
4+
* Per-AST identity map for native parser node wrappers.
5+
*
6+
* The native parser extension constructs a fresh WP_MySQL_Native_Parser_Node
7+
* every time it returns a child or descendant. Without an identity map, two
8+
* reads of the same logical node would yield distinct PHP objects, breaking
9+
* the WP_Parser_Node contract that callers can mutate a child in place and
10+
* see the mutation again when they walk the tree later.
11+
*
12+
* One cache is created lazily on the root node and shared by reference with
13+
* every wrapper interned through it. Lookup is keyed by the Rust-side
14+
* `node_index`, which is stable for the lifetime of the AST.
15+
*/
16+
class WP_MySQL_Native_AST_Cache {
17+
/**
18+
* Map of native node index => WP_MySQL_Native_Parser_Node.
19+
*
20+
* @var array<int, WP_MySQL_Native_Parser_Node>
21+
*/
22+
public $nodes = array();
23+
}

0 commit comments

Comments
 (0)