[OptionsResolver] Support union of types #59354
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey! Thanks for your PR. You are targeting branch "7.3" but it seems your PR description refers to branch "7.3 for features". Cheers! Carsonbot |
yceruto
requested changes
Jan 3, 2025
src/Symfony/Component/OptionsResolver/Tests/OptionsResolverTest.php
Outdated
Show resolved
Hide resolved
|
Also, we could find a regex that matches all possible variants instead of checking them one by one. |
Didn't find an easy one, but I'm opened to suggestion. I preferred to add the logic in the existing recursive implementation |
VincentLanglet
force-pushed
the
unionResolver
branch
from
c5fd1c9 to
f34466a
Compare
|
I found edge case where my previous implementation remove badly parenthesis, so I now introduce a private method |
yceruto
reviewed
Jan 3, 2025
src/Symfony/Component/OptionsResolver/Tests/OptionsResolverTest.php
Outdated
Show resolved
Hide resolved
|
Could you confirm that class Union is also supported with this new syntax? i.e. |
VincentLanglet
force-pushed
the
unionResolver
branch
from
5aed5da to
aca37dc
Compare
I added a test for |
VincentLanglet
force-pushed
the
unionResolver
branch
from
c43650c to
b108355
Compare
nicolas-grekas
approved these changes
Jan 6, 2025
yceruto
approved these changes
Jan 6, 2025
fabpot
approved these changes
Jan 6, 2025
|
Thank you @VincentLanglet. |
fabpot
added a commit
that referenced
this pull request
Jan 12, 2025
…hangelog (alamirault) This PR was merged into the 7.3 branch. Discussion ---------- [OptionsResolver] Add missing support of union type in changelog | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT I found changelog is missing while documenting symfony/symfony-docs#20531 Code PR: #59354 Commits ------- 946278f [OptionsResolver] Add missing support of union type
Merged
nicolas-grekas
added a commit
that referenced
this pull request
Jul 29, 2025
….9x faster (bendavies) This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [OptionsResolver] Optimize splitOutsideParenthesis() - 5.9x faster | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no <!-- if yes, also update src/**/CHANGELOG.md --> | Deprecations? | no <!-- if yes, also update UPGRADE-*.md and src/**/CHANGELOG.md --> | Issues | Fix #59354 <!-- prefix each issue number with "Fix #"; no need to create an issue if none exists, explain below --> | License | MIT This PR optimises the `splitOutsideParenthesis` method in `OptionsResolver.php`, achieving a 2.91x performance improvement. I discovered this method as a performance hotspot while benchmarking a large Symfony form with many fields. Profiling revealed that `splitOutsideParenthesis` was consuming a significant portion of the form processing time. The `splitOutsideParenthesis` method (introduced in [PR #59354](#59354)) is called frequently during options resolution and has several performance bottlenecks: 1. Character-by-character string concatenation creates new string objects on each iteration (particularly inefficient in PHP due to copy-on-write behavior) 2. All input strings are processed the same way, regardless of complexity - no fast path for simple types 3. Multiple conditional checks per character ## Test Methodology Here's how all performance measurements were conducted: - **Benchmark tool**: hyperfine (10 runs with 1 warmup run) - **Test iterations**: 100,000 iterations per test case - **Test data**: 16 different type patterns: - Simple types: `string`, `int`, `bool`, `array` - Union types: `string|int`, `string|int|bool`, `string|int|bool|array` - Parentheses types: `string|(int|bool)`, `(string|int)|bool` - Nested types: `string|(int|(bool|float))`, `(string|int)|(bool|float)` - Array types: `string[]`, `int[]` - Class types: `MyClass`, `\\Namespace\\Class` - Complex union: `string|int|bool|array|object|resource|callable` Each optimisation was tested in isolation to measure its individual impact, then all optimisations were combined for the final benchmark. ## Optimisations ### 1. Fast Path for Simple Types (No Pipes) Most type declarations are simple types like `string`, `int`, `MyClass`, etc. without any union types. **Implementation:** ```php if (!\str_contains($type, '|')) { return [$type]; } ``` ### 2. Fast Path for Union Types (No Parentheses) Common union types like `string|int|bool` don't need complex parsing - PHP's `explode()` is much faster. **Implementation:** ```php if (!\str_contains($type, '(') && !\str_contains($type, ')')) { return \explode('|', $type); } ``` ### 3. Eliminate String Concatenation String concatenation in loops creates memory overhead. Using `substr()` avoids creating intermediate strings. **Implementation:** ```php // Instead of: $currentPart .= $char; // Use: $parts[] = \substr($type, $start, $i - $start); ``` ### 4. Switch Statement Optimisation Eliminates Multiple conditional checks per character. **Implementation:** ```php switch ($char) { case '(': ++$parenthesisLevel; break; case ')': --$parenthesisLevel; break; case '|': // ... } ``` ## Benchmark Results ### Individual Optimisation Impact Testing each optimisation in isolation: ```bash hyperfine --warmup 1 --runs 10 \ --sort=command \ --reference 'php test_original.php' \ 'php test_opt1_fast_path_simple.php' \ 'php test_opt2_fast_path_union.php' \ 'php test_opt3_no_string_concat.php' \ 'php test_opt4_switch_statement.php' ``` ``` Relative speed comparison 1.00 php test_original.php 1.23 ± 0.02 php test_opt1_fast_path_simple.php 1.95 ± 0.04 php test_opt2_fast_path_union.php 1.13 ± 0.03 php test_opt3_no_string_concat.php 1.35 ± 0.03 php test_opt4_switch_statement.php ``` ### Combined Optimisation Impact Combining all optimisations: ```bash hyperfine --warmup 1 --runs 10 \ --sort=command \ --reference 'php test_original.php' \ 'php test_optimised.php' ``` ``` Relative speed comparison 1.00 php test_original.php 2.91 ± 0.03 php test_optimised.php ``` Commits ------- b568fef [OptionsResolver] Optimize splitOutsideParenthesis() - 5.9x faster
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support for
|syntax when describing allowedType in OptionResolver.It's not really useful for
int|stringsince we can pass an array['string', 'int'].But it's useful for array values since
(int|string)[]was not possible so far and['string[]', 'int[]']was not the same thing.