Allow disabling return value validation to improve performance - #1871
Allow disabling return value validation to improve performance#1871mahmudsudo wants to merge 17 commits into
Conversation
|
Please fix CI and address the Copilot review once it's available. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new trustResult flag intended to improve execution performance by skipping several runtime validations/serialization steps and “trusting” resolver return values.
Changes:
- Added a
trustResultboolean flag to theExecutorandGraphQLpublic APIs and threaded it through execution context. - Added
trustResultsupport toServerConfigand passed it through the server execution helper. - Added PHPUnit coverage for the new behavior and updated the generated class reference signatures.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Executor/TrustResultTest.php | Adds tests demonstrating which validations are skipped when trustResult=true. |
| src/Server/ServerConfig.php | Adds trustResult config option, setter, and getter. |
| src/Server/Helper.php | Passes trustResult from ServerConfig into GraphQL::promiseToExecute(). |
| src/GraphQL.php | Adds trustResult parameter to facade APIs and forwards it to the executor. |
| src/Executor/ReferenceExecutor.php | Skips non-null, leaf serialization, list iterable validation, and isTypeOf checks when trusted. |
| src/Executor/Executor.php | Adds trustResult parameter to executor APIs and forwards it into the implementation factory. |
| src/Executor/ExecutionContext.php | Stores trustResult in the execution context. |
| docs/class-reference.md | Updates documented method signatures and phpstan type aliases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@spawnia fixed ci errors and copilot reviews |
spawnia
left a comment
There was a problem hiding this comment.
I would not categorize this pull request as a fix - it is a new feature.
Have you benchmarked this?
|
I ran two sets of benchmarks. No regression when
|
| Scenario | Without trustResult |
With trustResult |
Improvement |
|---|---|---|---|
500 items × 8 built-in scalar fields (4 000 serialize() calls) |
32.98ms | 31.54ms | ~4.4% |
100 items × 5 custom scalar fields (preg_replace in serialize()) |
5.04ms | 4.57ms | ~9.4% |
100 objects with isTypeOf callback |
3.39ms | 3.14ms | ~7.3% |
| 30 orders with nested objects (combined) | 3.30ms | 3.24ms | ~1.8% |
The benefit scales with how expensive each serialize() / isTypeOf() call is. Built-in scalar serialization is cheap (simple type coercion), so gains there are modest; custom scalars or non-trivial isTypeOf checks show more improvement.
Benchmarks were run without opcache on PHP 8.3.6.
|
Ah, gotcha. 👍 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Executor/Executor.php:187
Executor::promiseToExecute()calls the configured implementation factory with 10 positional arguments. On PHP 8+ this will throw anArgumentCountErrorfor older userland factories that only declare the pre-existing parameters, contradicting the new test expecting extra args to be ignored. Consider invoking the factory with only the number of parameters it declares (unless variadic) to preserve BC while still passing$trustResultto newer factories.
$executor = (self::$implementationFactory)(
$promiseAdapter,
$schema,
$documentNode,
$rootValue,
$contextValue,
$variableValues ?? [],
$operationName,
$fieldResolver ?? self::$defaultFieldResolver,
$argsMapper ?? self::$defaultArgsMapper,
$trustResult,
);
| final class TrustResultTest extends TestCase | ||
| { | ||
| /** @see https://github.com/webonyx/graphql-php/issues/1493 */ | ||
| public function testTrustResultSkipsListIterableValidation(): void |
| if ($completed === null && ! $this->exeContext->trustResult) { | ||
| throw new InvariantViolation("Cannot return null for non-nullable field \"{$info->parentType}.{$info->fieldName}\"."); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/Executor/Executor.php:180
Executor::promiseToExecute()invokes the user-provided implementation factory with the new$trustResultargument unconditionally. Any existing custom factory that was defined with fewer parameters will throw an ArgumentCountError, which defeats the BC goal captured bytestCustomImplementationFactoryIgnoresExtraArguments(). Consider only passing as many arguments as the factory declares (unless it is variadic).
$executor = (self::$implementationFactory)(
$promiseAdapter,
$schema,
$documentNode,
$rootValue,
tests/Executor/TrustResultTest.php:15
- This test name says iterable validation is skipped, but the assertions verify that the iterable validation error is still present even when
trustResult=true. Rename the test to reflect the behavior being asserted.
public function testTrustResultSkipsListIterableValidation(): void
src/GraphQL.php:77
- The
trustResultoption description says it "skips normal type and serialization checks", but some runtime type invariants are still enforced (e.g. list fields must return an iterable, and abstract types still require valid runtime types). Tighten the wording to match the actual behavior to avoid misleading users.
* trustResult:
* When true, assumes resolver results are already correctly typed and serialized
* and skips normal type and serialization checks. This is purely for
* performance optimization. The tradeoff is potentially corrupted results for the client
* if resolvers return malformed data. Only enable this when you are confident
docs/class-reference.md:61
- The
trustResultoption description here matchessrc/GraphQL.phpand currently claims it "skips normal type and serialization checks", but the implementation still enforces some invariants (e.g. list fields must return iterable). Update the docs to match actual behavior.
* trustResult:
* When true, assumes resolver results are already correctly typed and serialized
* and skips normal type and serialization checks. This is purely for
* performance optimization. The tradeoff is potentially corrupted results for the client
* if resolvers return malformed data. Only enable this when you are confident
# Conflicts: # tests/Executor/ExecutorTest.php
closes #1493