Performance: Follow-up fixes to prepend_to_selector() optimization.#79499
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in df325f7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/28393619728
|
| } | ||
|
|
||
| /** | ||
| * @return array[] |
There was a problem hiding this comment.
| * @return array[] | |
| * @return array<string, array{selector: string, expected: string}> |
| } | ||
|
|
||
| /** | ||
| * @return array[] |
There was a problem hiding this comment.
| * @return array[] | |
| * @return array<string, array{selector: string, expected: string}> |
There was a problem hiding this comment.
added in 12f1e60, but used the expected: string[] type
| $selectors[] = $current_selector; | ||
| $current_selector = ''; | ||
| /* | ||
| * Start of a parenthesized expression, which maintains a stack of parenthesis. |
There was a problem hiding this comment.
| * Start of a parenthesized expression, which maintains a stack of parenthesis. | |
| * Start of a parenthesized expression, which maintains a stack of parentheses. |
| * | ||
| * // Comments stay with the selector they follow. | ||
| * array( '.a /* a, the first *\/', '.b' ) === self::split_selector_list( '.a /* a, the first *\/,.b' ); | ||
| * |
|
@westonruter I also auto-generated around 15 million selectors of varying counts (combined count), complexity, and syntax and found no cases where there were misparses. the testing may not be comprehensive and could have missed things, but I think it’s good |
12f1e60 to
abaa8c3
Compare
|
squashed in preparation to merge. former HEAD was 12f1e60 |
See #76556 A recent optimization to `theme.json` code adds a fast-path to avoid attempting to split a selector list by commas, assuming that the absence of a parentheses means that all remaining commas are comma-tokens. However, there are several other CSS syntax forms which are common in selectors which would present commas that are not comma tokens, and the new optimization will lead to CSS selector corruption. This patch expands the list of potential syntax-form-openers to exclude the remaining domain of selector inputs which would be broken by the fast-path, and applies fixes to `split_selector_list()` which would have left failing tests when applying the bug-fix to the earlier work. Co-Authored-By: Joseph Scott <josephscott@git.wordpress.org> Co-Authored-By: Weston Ruter <westonruter@git.wordpress.org>
abaa8c3 to
64f2669
Compare
In #79499 the `split_selector_list()` method was updated to avoid a number of mis-parsing cases with CSS selectors. The Core backport in WordPress/wordpress-develop#11857 includes a couple of updates not caught during review in Gutenberg, so this patch includes those updates: - the speedup from #76556 is added to `append_to_selector()` as well as `prepend_to_selector()`. - all selectors are trimmed, even single-selector selector lists and the last selector in a list. - callees no longer manually trim the results, improperly, leaning instead on the behavior in `split_selector_list()`.
Trac ticket: https://core.trac.wordpress.org/ticket/65533 This PR syncs the foloowing Gutenberg PRs: - WordPress/gutenberg#76556 - WordPress/gutenberg#79499 Opus 4.6 used in the initial patch creation in Gutenberg. Co-Authored-By: Dennis Snell <dmsnell@git.wordpress.org>
Splitting a CSS selector involves unnecessary computation and allocation when the selector could not possibly involve more than one selector. It was discovered in profiling that this function was being called 4000+ times in a single request, and adding a pre-condition before splitting saved meaningful time on a page render. This patch adds the pre-condition to verify that any commas present in a selector string can only be “comma tokens,” which split selectors at the top level, making it safe to fall back to simpler parsing that’s more efficient than general separator splitting. This commit synchronizes work from the following Gutenberg PRs: - WordPress/gutenberg#76556 - WordPress/gutenberg#79499 Developed in: #12306 Discussed in: https://core.trac.wordpress.org/ticket/65533 Follow-up to [62607]. Props dmsnell, josephscott. See #65533. git-svn-id: https://develop.svn.wordpress.org/trunk@62650 602fd350-edb4-49c9-b593-d223f7449a82
Splitting a CSS selector involves unnecessary computation and allocation when the selector could not possibly involve more than one selector. It was discovered in profiling that this function was being called 4000+ times in a single request, and adding a pre-condition before splitting saved meaningful time on a page render. This patch adds the pre-condition to verify that any commas present in a selector string can only be “comma tokens,” which split selectors at the top level, making it safe to fall back to simpler parsing that’s more efficient than general separator splitting. This commit synchronizes work from the following Gutenberg PRs: - WordPress/gutenberg#76556 - WordPress/gutenberg#79499 Developed in: WordPress/wordpress-develop#12306 Discussed in: https://core.trac.wordpress.org/ticket/65533 Follow-up to [62607]. Props dmsnell, josephscott. See #65533. Built from https://develop.svn.wordpress.org/trunk@62650 git-svn-id: http://core.svn.wordpress.org/trunk@61935 1a063a9b-81f0-0310-95a4-ce76da25c4cd
See #76556
A recent optimization to
theme.jsoncode adds a fast-path to avoid attempting to split a selector list by commas, assuming that the absence of a parentheses means that all remaining commas are comma-tokens.However, there are several other CSS syntax forms which are common in selectors which would present commas that are not comma tokens, and the new optimization will lead to CSS selector corruption.
This patch expands the list of potential syntax-form-openers to exclude the remaining domain of selector inputs which would be broken by the fast-path.