WordPress / performance

Performance plugin from the WordPress Performance Group, which is a collection of standalone performance modules.
https://wordpress.org/plugins/performance-lab/
GNU General Public License v2.0
364 stars 98 forks source link

Create Site Health Audit Enqueued Assets module #25

Closed ThierryA closed 2 years ago

ThierryA commented 2 years ago

Similar to #4 and #5, would be great to include to work started by @audrasjb here as module 😄

manuelRod commented 2 years ago

Shouldn't we define messaging? Maybe tailor it for end-users? Also, where this info is gonna show in the Site Health Screen? Maybe all modules should be added under a custom tab?

ThierryA commented 2 years ago

Shouldn't we define messaging? Maybe tailor it for end-users?

+1 to that

Also, where this info is gonna show in the Site Health Screen?

Stacked with all other items and with a "Performance" badge, although that is not set in stone I guess.

Meta note, raising awareness of too much CSS/JS etc is a good start, doing so and providing the source of the "offenders" is one step more helpful, doing so AND providing an action to solve the issue is the pinnacle. The later is extremely hard without risking to break something indeed.

cc @westonruter who has a lot of experience with sourcing offenders 😉

westonruter commented 2 years ago

Yeah, identifying the theme/plugin responsible for enqueuing a script or stylesheet is something the AMP plugin does heavily. It also goes to the extent of sourcing where inline scripts/styles are coming from (e.g. via wp_localize_script(), wp_add_inline_script(), and wp_set_script_translations(), and wp_add_inline_style()). Beyond WP_Scripts and WP_Styles, it also goes to the extent of identifying the source of scripts/styles that are printed manually, such as in wp_head or wp_footer. Not all of this will be applicable or important perhaps in the beginning, but I think it's all doable for core.

manuelRod commented 2 years ago

guys, I can work bringing https://github.com/audrasjb/site-health-audit-enqueued-assets/blob/master/site-health-audit-enqueued-assets.php to a module, and then we can discuss further messaging/features?

ThierryA commented 2 years ago

@manuelRod thanks for raising your hand. Sure you can either open a PR bringing the code in a module and we discuss further on it (I have some thoughts about the > 10 asset as a signal) or alternatively edit this issue description with more details about the approach and we continue discussing in this issue. Either war works 😄

manuelRod commented 2 years ago

@ThierryA here is the PR: https://github.com/WordPress/performance/pull/54

We can continue the conversation here.

So we are moving from 10 assets to total filesize, but should we maybe keep a combination of both? I think it is still useful to know the number of resources enqueued since we can still recommend concatenating them.

westonruter commented 2 years ago

If concatenation is recommended that would then probably also require recommending an optimization plugin that does such such concatenation. Otherwise, concatenation would only be something that could be done in the context of a theme/plugin's individual scripts. In this latter case, the call to action would be for the site owner to consider an alternative theme/plugin.

ThierryA commented 2 years ago

Continuing the comment thread conversation here. I would suggest to have a discussion with the broader group about the scripts and stylesheets size threshold as well as the number of instance if we decide to include this as a signal too.

https://almanac.httparchive.org/en/2021/javascript#how-much-javascript-do-we-load and https://almanac.httparchive.org/en/2021/css may help us understanding the current state of the web

We could base the threshold on CWV guidance if that resonate with everyone.

@felixarntz @adamsilverstein paging you here as well, would love more folks thoughts on this too.

adamsilverstein commented 2 years ago

Also relevant: the CMS section of the web almanac also breaks out some WordPress specific stats showing the high number of resources enqueued: https://almanac.httparchive.org/en/2021/cms#plugins

felixarntz commented 2 years ago

@manuelRod @ThierryA @adamsilverstein Looking at the two posts on JS and CSS usage above, one thing to realize is that (at least on desktop) the amount of JS and the overall file size of JS at the 75th percentile is about 3 times as much as the respective value for CSS.

I'm focusing on specifically the 75th percentile since that is where for CSS we are above 10 assets, which is where based on the PR right now we would warn the user. At the same (75th) percentile, the CSS file size is slightly above 100KB, so that may be a good relative value to use. For JS then, the 75th percentile has above 30 assets, and, at least on desktop, above 300KB. The value on mobile is a lot higher, but we should probably use the lower value as indicator for boundary.

There's somewhat two things we need to resolve here: We need to first figure out a relative relationship between the 4 metrics in how the thresholds should relate to each other, and then we need to determine what is the appropriate threshold to warn users. If we agree that warning users when they have more than 10 CSS assets enqueued is a good approach, a good approach might be the following for the 4 thresholds:

That's an initial idea, of course subject to discussion. While some of these numbers may seem high, we also don't want to make it "almost pointless" in that almost every site would then get warned. We can always lower the thresholds to become stricter later. Thoughts?

manuelRod commented 2 years ago

Thanks for the feedback @felixarntz. I think we should start somewhere and those 4 thresholds can be it. Maybe we need to work a little bit extra on the messaging we want to show, like, don't worry the webmaster excessively if they see a recommendation.

We need to address:

Current Strings: The amount of enqueued JS assets is acceptable. Your website enqueues %s scripts. Try to reduce the number of JS assets, or to concatenate them.

The amount of enqueued CSS assets is acceptable. Your website enqueues %s styles. Try to reduce the number of CSS assets, or to concatenate them.

manuelRod commented 2 years ago

@felixarntz @ThierryA I've implemented the above thresholds, these are the current string (pending to review them and decide a final version):

The amount of %1$s enqueued %2$s (size: %3$s) is acceptable. Your website enqueues %1$s %2$s (size: %3$s). Try to reduce the number or to concatenate them.

eclarke1 commented 2 years ago

@manuelRod @ThierryA could I recommend we move this issue to the 'In Progress' column in the Project Board to indicate it has an owner and is being worked on?

ThierryA commented 2 years ago

Thanks for the heads up @eclarke1, moved!

pacmanito commented 2 years ago

Hi, all. I wonder what's the logic beyond recomending staying under certain number of resources and using concatenation? I am used to think these are best practices from HTTP/1 era. Are there any tests supporting the idea that resources number has any meaningful impact on page loading? Concatenation can be even counterproductive, imagine user opening home page and getting concatenated resources, then he/she opens another page with slightly different assets set and concatendated file is downloaded again in full instead of a few missing small assets file.

manuelRod commented 2 years ago

Hello @pacmanito, I would say the messaging we have right now is just a provisional placeholder, we are actually deciding on it. The idea of this feature was just trying to give a sign to the web owner that it has either too many assets ( I think it is still useful ) or too heavy, in most cases one will lead to the other, and in most cases will be because the site has too many plugins installed.

See google pageSpeed report messaging:

Screen Shot 2022-01-14 at 9 51 11 AM

We could reuse something in these lines "Consider reducing, or switching, the number of WordPress plugins loading unused JavaScript in your page."

pacmanito commented 2 years ago

@manuelRod thank you for clarification. Maybe some further research is needed to give some clear recomendations? AFAIK concatenation was still superior in many cases 3-4 years ago (although the difference was negligible), but mostly due to poor browser and server implementations of HTTP/2. I suppose the situation has changed by now and no-concatenation configuration should be better, but this is only my guess.

manuelRod commented 2 years ago

@ThierryA @felixarntz did you have the chance to review the latest changes?

felixarntz commented 2 years ago

Opened #134 and #135 as follow-up issues, therefore closing this one. Our second module has landed 🎉

dainemawer commented 2 years ago

Just a quick insight on the thresholds. Not all sites are equal and to be honest, considering the versatility of WP environments, page builders etc - those thresholds are going to get gobbled up incredibly quickly. Starting a fresh theme is not equal to starting with 0kb of JS or CSS - wp-includes enqueues a ton of stuff by default.

It's a bit unfair on the user to assume that they would easily be able to control a threshold of a 100kb of CSS or 10 CSS assets when WP is already enqueuing 3 or 4 assets by default. Just installing Gravity Forms and rendering a form would enqueue a number of assets with poor code coverage and then your thresholds would be met.

What do we do in the case where we have a large site, maybe a high-traffic news site for instance? Those thresholds would more than likely be unrealistic. This is where Performance Budgets come into play - and in my opinion is probably an area that is beyond the scope of WP. I know Im playing a bit of devils advocate here but providing context is important. You would be hard-pressed to find major sites, or even minor sites running on WP with such low metrics. Im not advocating by any means for larger metrics here, but we do need to be realistic about the fact that we have no control over plugins or users.

Part of me leans to allowing users to add in their own thresholds, though that would more than likely make things redundant. Technically, you could include 100 scripts of 1KB and still be fine in terms of our thresholds above. In fact, that would still be more performant than 10 scripts of 10kb. Just some insight!

ThierryA commented 2 years ago

It terms of how realistic the current threshold is I tend to agree with the rational @dainemawer shared. With that said, the bottom line is that user experiencing a site doesn't care if it is a large site, if it has many plugins or whatever else is making a site slow, all they care about is their experience to consume the content of a site.

Perhaps this module should be experimental initially?

PS: personally I think that this Site Health audit is only a fragment of what should be surfaced to site owners. In an ideal world, performance health audits should be a lot less technical and much more actionable as the vast majority of WordPress Site Owners are non technical folks.

adamsilverstein commented 2 years ago

I tend to agree that these thresholds are a bit arbitrary and not readily user-actionable.

I'd be fine marking this feature as experimental, my only concern with that is fewer users are likely to test the feature then: they will have to manually enable the feature rather than it being auto-activated if we make it experimental. Also, there is no chance of some adverse behavior/degradation with this feature and the entire plugin is somewhat experimental, so I'm not sure the benefit of marking it experiemental.

felixarntz commented 2 years ago

+1 to marking the module as experimental, for the following reasons:

It's definitely a safer bet to go with that, and we can always change it to a non-experimental module in the future as it matures more. Compared to the other 3 modules already merged, this does feel a bit more "early" and less polished.

manuelRod commented 2 years ago

Sorry for being late the party,

To sum up, I see "experimental" as a different thing, but if we all agree, I'm in too.

felixarntz commented 2 years ago

Pasting my reply from https://github.com/WordPress/performance/pull/205#issuecomment-1059488955 in here, so that we can follow up on the conversation here as needed:

Indeed this module wouldn't break anyone's site, but IMO the main point for marking this as experimental is that it's still in an earlier stage of development compared to the other modules. There are still foundational iterations happening, e.g. we haven't fully defined what the thresholds should be, and the approach to gathering the assets is known to be not yet reliable for certain environments. We also need to think about how to make this warning more actionable, e.g. at a minimum tell users which plugins or themes are responsible for enqueuing the majority of assets etc.

As we work towards these enhancements and define the intended scope of the module further, eventually we could then remove the experimental label from it.