Profile memory usage and warn when available memory is insufficient

[westonruter]

A user reported a fatal error:

Allowed memory size of 33554432 bytes exhausted (tried to allocate 72 bytes) in .../wp-content/plugins/amp/vendor/sabberworm/php-css-parser/lib/Sabberworm/CSS/Parser.php on line 784

We need to check the memory usage of the AMP plugin with the core themes and common plugins (especially Jetpack) to see how much memory is normally required. Then we can detect how much memory is allocated to PHP in the AMP plugin, and when there is an insufficient amount we can either short-circuit the plugin from initializing (like we do for old PHP versions), or we can limit access to the Classic mode (which could still cause problems).

In the admin notice for users we can direct them to the Codex which has docs on Increasing memory allocated to PHP.

Relates to #1017.

[swissspidy]

Similar report: https://wordpress.org/support/topic/fatal-error-3144/

[westonruter]

Note the similar report was regarding another AMP plugin, not this one.

I just checked memory usage of the plugin while doing wp amp validate-site, as it checks most URLs of a site with the AMP validation routines which are much more expensive and memory intensive than normal runtime validation. On top of this, I prevented the parsed CSS from being cached, to better represent page generation from a cold state.

Of 59 URLs:

• Minimum peak memory: 2097152 bytes (2MB)
• Maximum peak memory: 10489856 bytes (10.5MB)
• Average peak memory: 6402117.424 bytes (6.40MB)
• Median peak memory: 6295552 bytes (6.3MB)

I got the data via quick and dirty mu-plugin:

add_action(
	'init',
	function() {
		register_shutdown_function(
			function () {
				$peak_memory_usage = memory_get_peak_usage( true );
				if ( isset( $_GET['amp'] ) && isset( $_GET['amp_validate'] ) ) {
					error_log( $peak_memory_usage );
				}
			}
		);
	}
);

And then I grabbed the peak memory usage from the error log.

All of this to say, the maximum peak memory when validating my entire local site was just over 10MB. This is naturally way less than WordPress's default 32MB memory. This being the case, I'm not sure we actually need to do anything here.

[westonruter]

This came up in another support topic. We may want to preemptively check the amount of CSS that is about to be parsed, and if it is massive and there is not enough available memory, then we should short-circuit perhaps with a new validation error related to there not being enough memory.

I suggest we figure out a rough estimate mapping the input CSS file and the actual memory usage to parse the CSS, and then incorporate this data into the plugin to prevent fatal errors related to this.

[amedina]

Once we determine the right threshold, we should probably error out at initialization time rather than default to classic mode. My concern with the latter approach is that there is an implicit "incentive loophole" by having a "way out" without doing anything for updating the memory limits.

[amedina]

It might be good to run some benchmarks (on synthetically generated sites), to and capture a comprehensive view of memory usage at different levels. Perhaps there is room for optimizing memory usage.

[westonruter]

Once we determine the right threshold, we should probably error out at initialization time rather than default to classic mode. My concern with the latter approach is that there is an implicit "incentive loophole" by having a "way out" without doing anything for updating the memory limits.

I think I determined above that the lowest memory usage is still sufficient for paired/native mode, so I don't think we'll have to go this route.

My thinking is that we'd treat huge stylesheets similar to stylesheets that exceed the 50KB budget: we'd just exclude them from processing. In that case, the behavior in paired mode would be to redirect to the non-AMP version if the validation error is rejected, and if accepted then the stylesheet would be omitted.

We wouldn't be able to determine at initialization time if there is not enough available memory for a given theme since is only during post-processing that we would be able to determine if there is CSS that is excessively large to parse. Similarly, the stylesheet could be included at any time by an activated plugin as well.

[amedina]

Regarding experimentation for determining the right threshold, what is the dependency between memory consumption and the characteristics of the markup being generated?

…

On Sun, Mar 31, 2019, 9:25 PM Weston Ruter ***@***.***> wrote: Once we determine the right threshold, we should probably error out at initialization time rather than default to classic mode. My concern with the latter approach is that there is an implicit "incentive loophole" by having a "way out" without doing anything for updating the memory limits. I think I determined above that the lowest memory usage is still sufficient for paired/native mode, so I don't think we'll have to go this route. My thinking is that we'd treat huge stylesheets similar to stylesheets that exceed the 50KB budget: we'd just exclude them from processing. In that case, the behavior in paired mode would be to redirect to the non-AMP version if the validation error is rejected, and if accepted then the stylesheet would be omitted. We wouldn't be able to determine at initialization time if there is not enough available memory for a given theme since is only during post-processing that we would be able to determine if there is CSS that is excessively large to parse. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1914 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAe46fYUr2BNYO67cpcjdjMZJIItXiefks5vcYpNgaJpZM4bXxw-> .

[westonruter]

@amedina The more markup and the more CSS, the more memory is required. If we are about to parse a stylesheet that we know is going to cause PHP-CSS-Parser to exceed the allowed memory, then we should skip it with an error.

I don't think there is any need for a threshold anymore, since the minimum amount of memory allocated to WordPress appears sufficient for serving AMP. In fact, I tried forcing the memory_limit to 16M which is half of the (apparent) default of 32MB, and the AMP plugin's post processor was able to generate the page including all of the CSS processing.

[amedina]

That sounds good. I asked about the correlation above because if a threshold is needed we would have to do some representative benchmarking of content handling to determine the right value. Glad to know we don't need to do that at the moment.

[swissspidy]

@westonruter Do we already try to increase memory limit and max timeout during runtime? Like WordPress core itself does in certain cases.

[westonruter]

We do not, but that is an interesting idea. If we do it, I think it should only be done during the “deep validation” request. So it could be done in the AMP_Validation_Manager::add_validation_error_sourcing() method.

But I don't think reports of errors have been related to validation process but rather just regular template rendering, so I'm not sure that this would make much of a difference.

[westonruter]

This just came up again: https://wordpress.org/support/topic/internal-server-error-when-validating-amp/#post-11797307

[schlessera]

With WP-CLI, one of the most common support requests was about the package manager (which is an embedded version of Composer) running out of memory.

We already had a common issue created to explain what is happening and how to solve it. However, most people didn't find this common issue, and create a new bug report on GH instead.

So what we did is add a shutdown handler to catch the out of memory condition and then in the error report explain what the most likely cause is: wp-cli/package-command#95

This has dropped the support requests regarding this to practically zero, as we provided the missing link from the error to the troubleshooting help.

[westonruter]

How would this shutdown handler interact with the output buffer callback?

[schlessera]

According to the docs, the shutdown handler would be called before the output buffer callback.

This means that we'd have to manually decide and code what happens in this case.
It gives us control, which is nice. However, we have to remember that we ran out of memory, so we can't allocate any new variables/objects.

This would certainly need some testing.