Skip to content

Commit b47b748

Browse files
adamzieladamziel
authored
Move native AST identity cache into the Rust extension (#392)
Stacked on #391. #391 restored `WP_Parser_Node` identity semantics by interning native child wrappers in PHP. That fixed correctness, but the PHP-side cache formed a retention cycle and added measurable cost to hit-heavy translator-style workloads. This PR moves native wrapper identity out of PHP object properties entirely: - `WP_MySQL_Native_Parser_Node` no longer stores `$native_ast`, `$native_node_index`, or a PHP-side identity-cache object. - Native bridge calls now pass the wrapper itself, e.g. `wp_sqlite_mysql_native_ast_get_children( $this )`. - The Rust extension keeps a thread-local registry keyed by the PHP wrapper object pointer. - Registry entries map wrapper pointer -> `(NativeAstState, node_index, is_materialized)`, and each AST keeps a node-index -> wrapper-pointer cache. - Cached wrapper hits return the existing PHP object by pointer with its refcount bumped; the cache does not own a PHP reference. - `__destruct()` releases a wrapper from the Rust registry. Materialization marks the wrapper as detached from native reads while leaving it discoverable from the parent cache as long as it is still live. That breaks the cycle that mattered here: PHP wrappers no longer strongly reference a native AST object, and Rust no longer strongly references PHP wrappers. PHP's cycle collector can collect wrapper graphs normally; destructors then clean up the Rust registry entries. Tokens remain un-interned. The public token API has no mutators, and no caller in this repo relies on token object identity. ## Perf numbers From the passing `Native AST Walk Perf` CI run on this head (`2d93be25f599c3c4482480a6ab644d61b9337b12`), comparing this PR to the native no-cache baseline (`codex/native-lazy-ast-facade`): | Scenario | This PR | Baseline | Duration delta | Peak memory delta | |---|---:|---:|---:|---:| | parse only | 1.2859s, 54,098 qps, 30.0MB | 1.2715s, 54,711 qps, 30.0MB | +1.1% | 0.0% | | walk x1 | 3.3265s, 20,912 qps, 48.0MB | 3.4191s, 20,346 qps, 60.5MB | -2.7% | -20.7% | | rewalk x10 | 8.6352s, 8,056 qps, 52.0MB | 16.9658s, 4,100 qps, 90.5MB | -49.1% | -42.5% | | reread x20 | 1.8618s, 37,364 qps, 30.0MB | 2.4322s, 28,602 qps, 38.0MB | -23.5% | -21.1% | | subtree x5 | 9.8274s, 7,078 qps, 48.0MB | 13.8921s, 5,007 qps, 64.5MB | -29.3% | -25.6% | The parse-only path is effectively unchanged. The repeated-access workloads this cache is meant to help are materially faster and use less peak memory. For context, the same CI run measured the pure-PHP path at: | Scenario | Pure PHP | |---|---:| | parse only | 13.5292s, 5,142 qps, 68.0MB | | walk x1 | 16.1332s, 4,312 qps, 70.0MB | ## Safety coverage The PR now includes native-extension tests for: - stable wrapper identity across repeated child/descendant reads; - no reflected `$native_ast` / `$native_node_index` properties on wrappers; - child mutations surviving repeat reads and parent materialization; - materialized children remaining discoverable from a still-native parent; - repeated parse/walk/drop loops staying memory-bounded; - dropping root and descendant wrappers reclaiming registry entries; - child wrappers outliving root variables without use-after-free; - overlapping AST lifetimes not corrupting each other; - mutation-before-drop and rewalk loops staying memory-bounded. CI smoke checks also assert the SQLite-driver and WordPress test-container paths select the native wrapper model and do not regress back to `$native_ast` storage. ## Test plan - [x] `cargo check` for `packages/php-ext-wp-mysql-parser` - [x] `cargo fmt --check` for `packages/php-ext-wp-mysql-parser` - [x] PHP lint on changed PHP files - [x] Focused native identity/cycle PHPUnit tests with the Rust extension loaded - [x] Full `packages/mysql-on-sqlite` PHPUnit suite with the Rust extension loaded - [x] Full GitHub Actions suite on PR head - [x] Native AST Walk Perf workflow on PR head
1 parent 2c07dac commit b47b748

9 files changed

Lines changed: 667 additions & 448 deletions

File tree

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

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,13 +144,31 @@ jobs:
144144
$parser = $driver->create_parser( "SELECT 1" );
145145
$parser->next_query();
146146
$ast = $parser->get_query_ast();
147-
$property = ( new ReflectionClass( $ast ) )->getProperty( "native_ast" );
148-
$property->setAccessible( true );
149-
$native_ast = $property->getValue( $ast );
150-
if ( ! is_object( $native_ast ) || "WP_MySQL_Native_Ast" !== get_class( $native_ast ) ) {
147+
if ( ! ( $ast instanceof WP_MySQL_Native_Parser_Node ) ) {
151148
fwrite( STDERR, "SQLite driver did not return a native-backed AST.\n" );
152149
exit( 1 );
153150
}
151+
$reflection = new ReflectionObject( $ast );
152+
if ( $reflection->hasProperty( "native_ast" ) || $reflection->hasProperty( "native_node_index" ) ) {
153+
fwrite( STDERR, "Native wrapper still stores Rust AST handle properties.\n" );
154+
exit( 1 );
155+
}
156+
$first = $ast->get_first_child_node();
157+
if ( ! ( $first instanceof WP_MySQL_Native_Parser_Node ) ) {
158+
fwrite( STDERR, "Native wrapper did not return a native-backed child node.\n" );
159+
exit( 1 );
160+
}
161+
if ( $first !== $ast->get_first_child_node() ) {
162+
fwrite( STDERR, "Native wrapper identity is not stable across reads.\n" );
163+
exit( 1 );
164+
}
165+
$synthetic = new WP_Parser_Node( 0, "synthetic" );
166+
$first->append_child( $synthetic );
167+
$same_first = $ast->get_first_child_node();
168+
if ( $same_first !== $first || ! in_array( $synthetic, $same_first->get_children(), true ) ) {
169+
fwrite( STDERR, "Materialized native wrapper was lost from the parent cache.\n" );
170+
exit( 1 );
171+
}
154172
'
155173
156174
- name: Run PHPUnit tests with SQLite driver using parser extension

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

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,31 @@ jobs:
3838
else
3939
TAG="version-${VERSION}"
4040
fi
41-
wget -O sqlite.tar.gz "https://sqlite.org/src/tarball/sqlite.tar.gz?r=${TAG}"
41+
SQLITE_SOURCE="https://sqlite.org/src/tarball/sqlite.tar.gz?r=${TAG}"
42+
SQLITE_MIRROR="https://github.com/sqlite/sqlite/archive/refs/tags/${TAG}.tar.gz"
43+
DOWNLOADED=0
44+
for url in "$SQLITE_SOURCE" "$SQLITE_MIRROR"; do
45+
for attempt in 1 2 3 4 5; do
46+
if wget -O sqlite.tar.gz "$url"; then
47+
DOWNLOADED=1
48+
break 2
49+
fi
50+
if [ "$attempt" -lt 5 ]; then
51+
sleep $(( attempt * 10 ))
52+
fi
53+
done
54+
done
55+
if [ "$DOWNLOADED" -ne 1 ]; then
56+
exit 1
57+
fi
4258
tar xzf sqlite.tar.gz
59+
if [ ! -d sqlite ]; then
60+
SQLITE_DIR=$(find . -maxdepth 1 -type d -name 'sqlite-*' | head -n 1)
61+
if [ -z "$SQLITE_DIR" ]; then
62+
exit 1
63+
fi
64+
mv "$SQLITE_DIR" sqlite
65+
fi
4366
cd sqlite
4467
./configure --prefix=/usr/local CFLAGS="-DSQLITE_ENABLE_COLUMN_METADATA -DSQLITE_ENABLE_FTS5 -DSQLITE_USE_URI -DSQLITE_ENABLE_JSON1" LDFLAGS="-lm"
4568
make -j$(nproc)

‎.github/workflows/wp-tests-phpunit-native-extension-setup.sh‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,14 +125,36 @@ $driver = new WP_PDO_MySQL_On_SQLite( 'mysql-on-sqlite:path=:memory:;dbname=wp;'
125125
$parser = $driver->create_parser( 'SELECT 1' );
126126
$parser->next_query();
127127
$ast = $parser->get_query_ast();
128-
$property = ( new ReflectionClass( $ast ) )->getProperty( 'native_ast' );
129-
$property->setAccessible( true );
130-
$native_ast = $property->getValue( $ast );
131128
132-
if ( ! is_object( $native_ast ) || 'WP_MySQL_Native_Ast' !== get_class( $native_ast ) ) {
129+
if ( ! ( $ast instanceof WP_MySQL_Native_Parser_Node ) ) {
133130
fwrite( STDERR, "WordPress PHP test container did not select the native-backed AST.\n" );
134131
exit( 1 );
135132
}
133+
134+
$reflection = new ReflectionObject( $ast );
135+
if ( $reflection->hasProperty( 'native_ast' ) || $reflection->hasProperty( 'native_node_index' ) ) {
136+
fwrite( STDERR, "Native wrapper still stores Rust AST handle properties.\n" );
137+
exit( 1 );
138+
}
139+
140+
$first = $ast->get_first_child_node();
141+
if ( ! ( $first instanceof WP_MySQL_Native_Parser_Node ) ) {
142+
fwrite( STDERR, "Native wrapper did not return a native-backed child node.\n" );
143+
exit( 1 );
144+
}
145+
146+
if ( $first !== $ast->get_first_child_node() ) {
147+
fwrite( STDERR, "Native wrapper identity is not stable across reads.\n" );
148+
exit( 1 );
149+
}
150+
151+
$synthetic = new WP_Parser_Node( 0, 'synthetic' );
152+
$first->append_child( $synthetic );
153+
$same_first = $ast->get_first_child_node();
154+
if ( $same_first !== $first || ! in_array( $synthetic, $same_first->get_children(), true ) ) {
155+
fwrite( STDERR, "Materialized native wrapper was lost from the parent cache.\n" );
156+
exit( 1 );
157+
}
136158
EOF
137159

138160
node tools/local-env/scripts/docker.js run --rm php php -m | grep -qx 'wp_mysql_parser'

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
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';
3130
require_once __DIR__ . '/mysql/native/class-wp-mysql-native-parser-node.php';
3231
require_once __DIR__ . '/mysql/native/class-wp-mysql-parser.php';
3332
} else {

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

Lines changed: 0 additions & 23 deletions
This file was deleted.

0 commit comments

Comments
 (0)