Skip to content

Optimize Visitor::visitInParallel#1953

Open
OpaqueRock wants to merge 2 commits into
webonyx:masterfrom
OpaqueRock:optimize-visitor
Open

Optimize Visitor::visitInParallel#1953
OpaqueRock wants to merge 2 commits into
webonyx:masterfrom
OpaqueRock:optimize-visitor

Conversation

@OpaqueRock

Copy link
Copy Markdown

Split out from #1948, so the Visitor optimization can be reviewed independently of the Lexer::readString optimization (which remains in #1948).

Problem

Visitor::visitInParallel() builds an enter/leave dispatcher closure that is invoked once per AST node. On every call, for every wrapped visitor, it called extractVisitFn() to re-resolve the enter/leave callback for that visitor and the current node's kind — an O(nodes × visitors) number of array lookups. However, a callback's identity for a given (visitor, kind, isLeaving) triple never changes during a single traversal, so this work was being needlessly repeated for every node of the same kind.

Additionally, the dispatcher closures used func_get_args() plus argument unpacking ($fn(...func_get_args())) even though they are always invoked with the same five arguments, which carries avoidable per-call overhead.

Change

  • Cache the resolved enter/leave callback per visitor index and node kind, keyed lazily on first use (rather than prefilled from NodeKind's fixed set), using false as a "no callback" sentinel so ?? can distinguish "not yet cached" from "cached, nothing to call." Building the cache lazily also means visitors for custom (non-NodeKind) Node kinds are cached and dispatched correctly, same as before.
  • Replace func_get_args() / argument unpacking in the enter/leave closures with the five explicit, typed parameters ($node, $key, $parent, array $path, array $ancestors) that match Visitor::visit()'s single call site.

No public API or behavior changes; this is purely an internal performance optimization of visitInParallel()'s dispatch path.

Testing

  • composer test — full suite passes.
  • composer stan — no new errors.
  • composer rector / composer php-cs-fixer — no changes required.

extractVisitFn() re-derives the same enter/leave callable via array
lookups on every node, for every wrapped visitor - O(nodes x visitors)
calls - even though its result for a given (visitor, kind, isLeaving)
triple never changes during a traversal. Cache it lazily per
visitor/kind on first use, using `false` as a "no callback" sentinel,
so custom (non-NodeKind) Node kinds are also supported.

Also replace func_get_args() plus argument unpacking in the enter/leave
dispatcher closures with the five explicit, typed parameters they are
always called with, avoiding func_get_args()'s per-call overhead.
OpaqueRock added a commit to OpaqueRock/graphql-php that referenced this pull request Jul 22, 2026
The Visitor::visitInParallel() enter/leave callback caching
optimization has been split out into a separate PR
(webonyx#1953), so it can be reviewed independently of the
Lexer::readString optimization here.
@spawnia
spawnia requested a review from Copilot July 23, 2026 07:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves traversal performance in Visitor::visitInParallel() by avoiding repeated callback resolution and avoiding per-node func_get_args() overhead in the parallel visitor dispatch path.

Changes:

  • Adds lazy per-(visitor index, node kind) caching for resolved enter/leave callbacks (with false sentinel for “no callback”).
  • Replaces argument unpacking via func_get_args() with explicit dispatcher parameters to match the Visitor::visit() call site.
Comments suppressed due to low confidence (1)

src/Language/Visitor.php:452

  • Same as enter: making leave require 5 parameters can break callers that invoke the dispatcher directly with only (Node $node). Consider defaulting the extra parameters to keep the callable usable in both 1-arg and 5-arg contexts.
            'leave' => static function (Node $node, $key, $parent, array $path, array $ancestors) use ($visitors, $skipping, $visitorsCount, &$leaveFns) {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Language/Visitor.php Outdated
Comment thread src/Language/Visitor.php
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants