Size Change: +121 B (0%)

Total Size: 3.09 MB

_{compressed-size-action}

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 props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @aileenf.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: aileenf.

Co-authored-by: tellthemachines <[email protected]>
Co-authored-by: andrewserong <[email protected]>
Co-authored-by: Suhel-Shaikh-Mohammad <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Thanks for reviewing and testing @andrewserong! I think I've fixed all the issues now: accounting for gap values possibly being an array, checking it's the correct array on the PHP side, and making the columns computation work with 0 values.

do you have some repro steps I can use to verify the difference between trunk and this PR? I.e. some block markup and theme.json values to try?

I'm testing with a wide-aligned Grid, with both column count and min column width set:

<!-- wp:group {"align":"wide","layout":{"type":"grid","columnCount":3,"minimumColumnWidth":"10rem"}} -->
<div class="wp-block-group alignwide"><!-- wp:paragraph {"backgroundColor":"accent-1"} -->
<p class="has-accent-1-background-color has-background">content</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"accent-1"} -->
<p class="has-accent-1-background-color has-background">content</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"accent-1"} -->
<p class="has-accent-1-background-color has-background">content</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"accent-1"} -->
<p class="has-accent-1-background-color has-background">content</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"accent-1"} -->
<p class="has-accent-1-background-color has-background">content</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

and TT5 with a custom top-level blockGap value. I've also checked empty theme with a custom blockGap and it seems to work there too.

Flaky tests detected in 9a682d4.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/21192329945
📝 Reported issues:

• [Flaky Test] should insert two subsequent mentions #71871 in /test/e2e/specs/editor/various/mentions.spec.js

This PR seems to have caused a severe performance regression on the "loadPages" metric. You can see it in the performance job that run on the PR here or on the previous codevitals dashboard here. It seems worth investigating.

@youknowriad do we have any docs around how to read those perf tests? What does 1947.42 ms +7.5% -26.44% even mean? 😅
I'd also expect the perf tests to fail or at least emit a warning if a metric changes significantly like that. Otherwise what's the point of running them on each PR?

I wonder if we could add something like static $global_styles = null; at the top of the function and then where we call gutenberg_get_global_styles check that $global_styles is null before calling it and storing it in $global_styles? That way only the first instance of calling the function would grab the global styles data?

I'm not sure the regression is on the PHP side because the place Riad pointed to is looking at how fast the site editor loads. But as I said, those tests are very hard to read 🤷

Fwiw the loadPages numbers seem to be down again by a fair bit in the latest merged PR: https://github.com/WordPress/gutenberg/actions/runs/21195053819

Fwiw the loadPages numbers seem to be down again by a fair bit in the latest merged PR: https://github.com/WordPress/gutenberg/actions/runs/21195053819

Did that PR contain the changes in this PR? If it wasn't rebased before landing, then those results might be from a state before this PR landed 🤔

That way only the first instance of calling the function would grab the global styles data?

We are only calling the function once.

The layout code runs for every block on the page that has layout though, and for a complex template that might be a lot of blocks. And global styles will be exactly the same for each of those blocks, so is there a way we can store them somewhere and access them when needed, instead of fetching them for each block?

Did that PR contain the changes in this PR?

Oh good point, it probably didn't.

so is there a way we can store them somewhere and access them when needed, instead of fetching them for each block?

Yes, that's what I meant in my comment above. PHP's static variables is a neat feature that means you can have variables within a function where the state lives on beyond the first call. There's a few examples elsewhere in Gutenberg and WP that use that pattern: https://www.php.net/manual/en/language.variables.scope.php#language.variables.scope.static

Yes, that's what I meant in my comment above.

Oh I see, yes! I guess we could try that.

Let's see what the perf tests say: #74828

Looks like #74828 fixed up the performance issue for loadPages! 🎉

┌──────────────────────┬──────────────────────────────────────────┬─────────────────────────────┬───────────┐
│ (index)              │ 590872874d7c9a190c8ffd4a6397b1bc454bf876 │ trunk                       │ % Change  │
├──────────────────────┼──────────────────────────────────────────┼─────────────────────────────┼───────────┤
│ serverResponse       │ '408.05 ms +0.43% -8.44%'                │ '497.05 ms +0.25% -0.79%'   │ '-17.91%' │
│ firstPaint           │ '347.45 ms +4.91% -1.05%'                │ '339.35 ms +1.93% -8.77%'   │ '2.39%'   │
│ domContentLoaded     │ '1123.3 ms +2.08% -1.24%'                │ '1095.65 ms +1.56% -0.81%'  │ '2.52%'   │
│ loaded               │ '1123.8 ms +2.06% -1.21%'                │ '1096.15 ms +1.56% -0.83%'  │ '2.52%'   │
│ firstContentfulPaint │ '1279.4 ms +11.42% -1.31%'               │ '1365.35 ms +8.13% -9.63%'  │ '-6.3%'   │
│ firstBlock           │ '4070.05 ms +0.23% -1.35%'               │ '3959.6 ms +0.47% -0.34%'   │ '2.79%'   │
│ type                 │ '11.43 ms +9.89% -1.31%'                 │ '11.65 ms +6.61% -3.78%'    │ '-1.89%'  │
│ navigate             │ '29.24 ms +14.53% -1.92%'                │ '33.53 ms +6.92% -2.24%'    │ '-12.79%' │
│ loadPatterns         │ '1772.68 ms +2.73% -3.9%'                │ '1787.08 ms +7.63% -11.57%' │ '-0.81%'  │
│ loadPages            │ '1267.46 ms +1.74% -4.23%'               │ '1810.42 ms +2.4% -5.28%'   │ '-29.99%' │
│ wpTotal              │ '398.95 ms +0.45% -8.63%'                │ '488.02 ms +0.3% -0.79%'    │ '-18.25%' │
│ wpMemoryUsage        │ '11.74 MB +0% -0%'                       │ '11.73 MB +0% -0%'          │ '0.01%'   │
│ wpDbQueries          │ '53 +0% -0%'                             │ '53 +0% -0%'                │ '0%'      │
└──────────────────────┴──────────────────────────────────────────┴─────────────────────────────┴───────────┘

Thank you all

@youknowriad I'd still like to know the answers to my questions above. It doesn't feel helpful that the perf tests aren't flagging potential issues for PRs in any way, and some docs on how to read them would also be useful.

do we have any docs around how to read those perf tests? What does 1947.42 ms +7.5% -26.44% even mean? 😅

We don't. I think @jsnajdr added some of these numbers, I think one of them is variance and the other is the difference between the two commits/branches.

I'd also expect the perf tests to fail or at least emit a warning if a metric changes significantly like that. Otherwise what's the point of running them on each PR?

The main point is being able to confirm the regressions that are identified using the graph and for PR authors to check the impact of their PRs (some PRs obviously impact performance and need to be checked). That said, I do agree having a comment on the PR at least would be a good addition. I'm not sure entirely about failing because performance tests have a margin of error that unfortunately we can't get rid of fully but maybe we can have some thresholds. Feel free to explore it.

I think @jsnajdr added some of these numbers, I think one of them is variance and the other is the difference between the two commits/branches.

Yes, I added these stats in #60950, but they mean something different 🙂 When the serverResponse stat is reported as

408.05 ms +0.43% -8.44%

then the 408.05 ms value is a median of 10+ measurements, it's not a very precise number. The percentages say that most of the measurements were between "408.05ms minus 8.44 percent" and "408.05ms plus 0.43 percent". Gives you some picture about how sure we really are about the serverResponse value. It's not really a point, but rather a smeared vertical bar. In technical lingo these numbers are the 25%, 50% and 75% quartiles.

Now this is very useful when evaluating what the -17.91% change really means. Is it a real improvement or just a random fluctuation? The randomness is in the order of -8%, the -17.9% change is more than twice than that, and that means the improvement is real. On the other hand, if the change was just -5%, that's well within the -8% range and we probably didn't really achieve a 5% improvement.

I admit this is not intuitive or obvious at all. Happy to document it somewhere in a way that is comprehensible.