Skip to content

Commit 2b595f3

Browse files
authored
Restore instanceof WP_Parser for the native parser (#393)
Addresses Jan's note on #381: in native mode, `new WP_MySQL_Parser(...) instanceof WP_Parser` returns false because the native-mode class extends the Rust-registered `WP_MySQL_Native_Parser`, which has no `WP_Parser` in its chain. Existing downstream code doing `if ($parser instanceof WP_Parser)` silently skipped the parser whenever the extension was loaded. This restores the contract by always extending the pure-PHP `WP_Parser` and pulling the native-mode behaviour in via a trait: ```php class WP_MySQL_Parser extends WP_Parser { use WP_MySQL_Native_Parser_Impl; } ``` `WP_MySQL_Native_Parser_Impl` owns the composed `WP_MySQL_Native_Parser` instance and the four-method delegation surface (`parse`, `next_query`, `get_query_ast`, `reset_tokens`). `WP_Parser`'s protected state (`$grammar`, `$tokens`, `$position`) is initialised by `parent::__construct` and stays inert — the trait's overrides never read it. Adding a public method later means adding it to the trait — the class file itself is two lines and doesn't need touching. ## Why a trait, not a private property? A bare property would also work, but the trait keeps the class file expressing only the routing decision (`extends WP_Parser` + `use Rust_Implementation;`). The implementation lives in one place, symmetric to where a future PHP-mode trait could live if we ever want to mirror the structure. Behaviour-wise the two are equivalent. ## Performance The trait adds **one extra method-call frame per public-API call**. The public API is `parse()`, `next_query()`, `get_query_ast()`, `reset_tokens()` — called once per query. The actual parsing work happens inside the native call, so the delegation overhead is a small constant per query, not a multiplier on the parsing work. The `Parser Delegation Perf` workflow runs `tests/tools/run-parser-benchmark.php` (parses the full MySQL server-suite corpus, ~70k queries) three times on this PR and three times on the PR base, on the same runner, with the extension loaded both times. The comparison goes into the job summary on every push. ## Test plan - [x] PHP-only PHPUnit suite passes. - [ ] PHPUnit suite passes with the native extension loaded; the new `WP_MySQL_Parser_Instanceof_Tests` confirm `instanceof WP_Parser` and `instanceof WP_MySQL_Parser` both hold. - [ ] `Parser Delegation Perf` workflow shows the delegation cost is within noise.
1 parent 71a250a commit 2b595f3

5 files changed

Lines changed: 254 additions & 1 deletion

File tree

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
name: Parser Delegation Perf
2+
3+
# Measures the wall-time cost of routing the public parser API through a
4+
# composed `WP_MySQL_Native_Parser` instance (this PR) instead of letting
5+
# `WP_MySQL_Parser` directly extend the Rust class (PR base). The parse
6+
# benchmark walks the full MySQL server-suite corpus on both branches —
7+
# the delta is the per-call delegation cost.
8+
9+
on:
10+
push:
11+
paths:
12+
- '.github/workflows/parser-delegation-perf.yml'
13+
- 'packages/mysql-on-sqlite/src/mysql/native/**'
14+
- 'packages/mysql-on-sqlite/tests/tools/run-parser-benchmark.php'
15+
- 'packages/php-ext-wp-mysql-parser/**'
16+
pull_request:
17+
paths:
18+
- '.github/workflows/parser-delegation-perf.yml'
19+
- 'packages/mysql-on-sqlite/src/mysql/native/**'
20+
- 'packages/mysql-on-sqlite/tests/tools/run-parser-benchmark.php'
21+
- 'packages/php-ext-wp-mysql-parser/**'
22+
workflow_dispatch:
23+
24+
concurrency:
25+
group: ${{ github.workflow }}-${{ github.ref }}
26+
cancel-in-progress: true
27+
28+
jobs:
29+
perf:
30+
name: Parser delegation benchmark
31+
runs-on: ubuntu-latest
32+
timeout-minutes: 25
33+
34+
steps:
35+
- name: Checkout repository
36+
uses: actions/checkout@v4
37+
38+
- name: Set up PHP
39+
uses: shivammathur/setup-php@v2
40+
with:
41+
php-version: '8.2'
42+
coverage: none
43+
44+
- name: Set up Rust
45+
uses: dtolnay/rust-toolchain@stable
46+
47+
- name: Install native build dependencies
48+
run: |
49+
sudo apt-get update
50+
sudo apt-get install -y libclang-dev
51+
echo "PHP_CONFIG=$(command -v php-config)" >> "$GITHUB_ENV"
52+
LIBCLANG_SO="$(find /usr/lib -name 'libclang.so*' | head -n 1)"
53+
echo "LIBCLANG_PATH=$(dirname "$LIBCLANG_SO")" >> "$GITHUB_ENV"
54+
55+
- name: Install Composer dependencies (root)
56+
uses: ramsey/composer-install@v3
57+
with:
58+
ignore-cache: "yes"
59+
composer-options: "--optimize-autoloader"
60+
61+
- name: Install Composer dependencies (mysql-on-sqlite)
62+
uses: ramsey/composer-install@v3
63+
with:
64+
working-directory: packages/mysql-on-sqlite
65+
ignore-cache: "yes"
66+
composer-options: "--optimize-autoloader"
67+
68+
- name: Download MySQL test query corpus
69+
working-directory: packages/mysql-on-sqlite
70+
run: bash tests/tools/mysql-download-tests.sh || true
71+
72+
- name: Build parser extension (release)
73+
run: cargo build --release
74+
working-directory: packages/php-ext-wp-mysql-parser
75+
76+
- name: Locate built extension
77+
run: |
78+
EXT="$GITHUB_WORKSPACE/packages/php-ext-wp-mysql-parser/target/release/libwp_mysql_parser.so"
79+
test -f "$EXT" || { echo "Extension not built at $EXT"; exit 1; }
80+
echo "NATIVE_EXT=$EXT" >> "$GITHUB_ENV"
81+
82+
- name: Benchmark — this PR (trait delegation)
83+
working-directory: packages/mysql-on-sqlite
84+
run: |
85+
for i in 1 2 3; do
86+
echo "=== run $i ==="
87+
php -d extension="$NATIVE_EXT" tests/tools/run-parser-benchmark.php
88+
done | tee "$GITHUB_WORKSPACE/this-pr.txt"
89+
90+
- name: Check out baseline (PR base, direct extends)
91+
run: |
92+
git fetch --no-tags --depth=1 origin codex/native-lazy-ast-facade
93+
git worktree add ../baseline FETCH_HEAD
94+
95+
- name: Install Composer dependencies (baseline)
96+
uses: ramsey/composer-install@v3
97+
with:
98+
working-directory: ../baseline/packages/mysql-on-sqlite
99+
ignore-cache: "yes"
100+
composer-options: "--optimize-autoloader"
101+
102+
- name: Build baseline parser extension (release)
103+
run: cargo build --release
104+
working-directory: ../baseline/packages/php-ext-wp-mysql-parser
105+
106+
- name: Stage corpus into baseline
107+
run: |
108+
mkdir -p ../baseline/packages/mysql-on-sqlite/tests/mysql/data
109+
cp packages/mysql-on-sqlite/tests/mysql/data/*.csv \
110+
../baseline/packages/mysql-on-sqlite/tests/mysql/data/ 2>/dev/null || true
111+
112+
- name: Benchmark — baseline (direct extends)
113+
working-directory: ../baseline/packages/mysql-on-sqlite
114+
run: |
115+
BASE_EXT="$(realpath ../../packages/php-ext-wp-mysql-parser/target/release/libwp_mysql_parser.so)"
116+
for i in 1 2 3; do
117+
echo "=== run $i ==="
118+
php -d extension="$BASE_EXT" tests/tools/run-parser-benchmark.php
119+
done | tee "$GITHUB_WORKSPACE/baseline.txt"
120+
121+
- name: Summarize
122+
if: always()
123+
run: |
124+
{
125+
echo '### Parser delegation perf'
126+
echo
127+
echo '#### This PR (`use WP_MySQL_Native_Parser_Impl;`)'
128+
echo '```'
129+
cat "$GITHUB_WORKSPACE/this-pr.txt" 2>/dev/null || echo 'no output'
130+
echo '```'
131+
echo
132+
echo '#### Baseline (`extends WP_MySQL_Native_Parser`)'
133+
echo '```'
134+
cat "$GITHUB_WORKSPACE/baseline.txt" 2>/dev/null || echo 'no output'
135+
echo '```'
136+
} >> "$GITHUB_STEP_SUMMARY"
137+
138+
- name: Upload raw output
139+
if: always()
140+
uses: actions/upload-artifact@v4
141+
with:
142+
name: parser-delegation-perf-${{ github.run_id }}
143+
path: |
144+
this-pr.txt
145+
baseline.txt
146+
if-no-files-found: warn

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
if ( class_exists( 'WP_MySQL_Native_Parser', false ) ) {
2929
require_once __DIR__ . '/mysql/native/mysql-rust-bridge.php';
3030
require_once __DIR__ . '/mysql/native/class-wp-mysql-native-parser-node.php';
31+
require_once __DIR__ . '/mysql/native/trait-wp-mysql-native-parser-impl.php';
3132
require_once __DIR__ . '/mysql/native/class-wp-mysql-parser.php';
3233
} else {
3334
require_once __DIR__ . '/mysql/class-wp-mysql-parser.php';
Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
11
<?php
22

3-
class WP_MySQL_Parser extends WP_MySQL_Native_Parser {}
3+
/**
4+
* Native-mode public parser entry point.
5+
*
6+
* Always extends the pure-PHP `WP_Parser` so `$parser instanceof WP_Parser`
7+
* keeps working for callers regardless of whether the Rust extension is
8+
* loaded. The actual parsing work is delegated to a composed
9+
* `WP_MySQL_Native_Parser` instance via the `WP_MySQL_Native_Parser_Impl`
10+
* trait — see that file for the per-method delegation.
11+
*/
12+
class WP_MySQL_Parser extends WP_Parser {
13+
use WP_MySQL_Native_Parser_Impl;
14+
}
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
<?php
2+
3+
/**
4+
* Native-mode `WP_MySQL_Parser` implementation, delivered as a trait.
5+
*
6+
* The class that uses this trait (`WP_MySQL_Parser` in native mode)
7+
* extends the pure-PHP `WP_Parser` so callers' `instanceof WP_Parser`
8+
* checks keep working, while the actual parsing work is delegated to
9+
* the Rust-registered `WP_MySQL_Native_Parser` instance held in
10+
* `$this->native`. `WP_Parser`'s state (`$grammar`, `$tokens`,
11+
* `$position`) stays inert in native mode — the trait's overrides
12+
* never read it.
13+
*
14+
* Adding a public method here is enough to plumb a new public method
15+
* through to the native parser; the using class does not need touching.
16+
*/
17+
trait WP_MySQL_Native_Parser_Impl {
18+
/**
19+
* @var WP_MySQL_Native_Parser
20+
*/
21+
private $native;
22+
23+
/**
24+
* @param WP_Parser_Grammar $grammar
25+
* @param array<WP_Parser_Token>|WP_MySQL_Native_Token_Stream $tokens
26+
*/
27+
public function __construct( WP_Parser_Grammar $grammar, $tokens ) {
28+
// WP_Parser's `array $tokens` constructor signature can't accept
29+
// the native token stream object; its `$this->tokens` /
30+
// `$this->position` state is inert in native mode anyway, so we
31+
// pass an empty array to satisfy the parent contract and keep
32+
// the actual tokens on the native parser.
33+
parent::__construct( $grammar, array() );
34+
$this->native = new WP_MySQL_Native_Parser( $grammar, $tokens );
35+
}
36+
37+
/**
38+
* @param array<WP_Parser_Token>|WP_MySQL_Native_Token_Stream $tokens
39+
*/
40+
public function reset_tokens( $tokens ): void {
41+
$this->native->reset_tokens( $tokens );
42+
}
43+
44+
public function next_query(): bool {
45+
return $this->native->next_query();
46+
}
47+
48+
public function get_query_ast(): ?WP_Parser_Node {
49+
return $this->native->get_query_ast();
50+
}
51+
52+
public function parse() {
53+
return $this->native->parse();
54+
}
55+
}
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<?php
2+
3+
use PHPUnit\Framework\TestCase;
4+
5+
/**
6+
* `WP_MySQL_Parser instanceof WP_Parser` must hold in both modes.
7+
*
8+
* Pre-PR, the native-mode `WP_MySQL_Parser` was `extends WP_MySQL_Native_Parser`
9+
* (a Rust-registered class with no `WP_Parser` in its chain), so existing
10+
* downstream code doing `if ($parser instanceof WP_Parser)` silently
11+
* skipped the parser when the extension was loaded. This test pins the
12+
* contract for both modes.
13+
*/
14+
class WP_MySQL_Parser_Instanceof_Tests extends TestCase {
15+
16+
public function test_parser_is_instance_of_wp_parser(): void {
17+
$grammar = new WP_Parser_Grammar( include __DIR__ . '/../../../src/mysql/mysql-grammar.php' );
18+
$lexer = new WP_MySQL_Lexer( 'SELECT 1' );
19+
$tokens = $lexer instanceof WP_MySQL_Native_Lexer
20+
? $lexer->native_token_stream()
21+
: $lexer->remaining_tokens();
22+
$parser = new WP_MySQL_Parser( $grammar, $tokens );
23+
24+
$this->assertInstanceOf( WP_Parser::class, $parser );
25+
$this->assertInstanceOf( WP_MySQL_Parser::class, $parser );
26+
}
27+
28+
public function test_parser_returns_an_ast(): void {
29+
$grammar = new WP_Parser_Grammar( include __DIR__ . '/../../../src/mysql/mysql-grammar.php' );
30+
$lexer = new WP_MySQL_Lexer( 'SELECT 1 + 2' );
31+
$tokens = $lexer instanceof WP_MySQL_Native_Lexer
32+
? $lexer->native_token_stream()
33+
: $lexer->remaining_tokens();
34+
$parser = new WP_MySQL_Parser( $grammar, $tokens );
35+
36+
$ast = $parser->parse();
37+
$this->assertNotNull( $ast );
38+
$this->assertInstanceOf( WP_Parser_Node::class, $ast );
39+
}
40+
}

0 commit comments

Comments
 (0)