Optimize AMP_Tag_And_Attribute_Sanitizer::get_missing_mandatory_attributes()

[schlessera]

Summary

While doing an initial test run with Blackfire (see #3361), I noticed the following:

Of the 2.61s AMP page generation time, the AMP_Tag_And_Attribute_Sanitizer::get_missing_mandatory_attributes() method was accounting for more than 50% of the execution time.

It was calling:

• 58262x into substr()
• 58262x into DOMElement::hasAttribute()
• 57606x into AMP_Tag_And_Attribute_Sanitizer::check_attr_spec_rule_mandatory()

As you can see from the flamegraph, the AMP generation is disproportionally large because of that:

The problem is that the method loops over all nodes, and then for each of them loops over all specs, and then checks whether something is mandatory.

The current PR addresses this by extracting one conditional out of AMP_Tag_And_Attribute_Sanitizer::check_attr_spec_rule_mandatory() and puts it as the first check within AMP_Tag_And_Attribute_Sanitizer::get_missing_mandatory_attributes(). This check is the most important one: is this spec mandatory? The entire rest of the logic can be skipped if that is not the case.

Here's the result:

The method has dropped out of the captured graph of Blackfire, meaning it accounts for less than 1% of the execution time in any way. To get a more detailed overview, I'd have to re-run with blackfire curl --debug to disable this pruning, but it doesn't allow me to do that right now it seems.

Anyway, the function went from being more than 50% of the total page generation time, to being insignificant.

The flamegraph also looks completely different now:

Checklist

• My pull request is addressing an open issue (please create one otherwise).
• My code is tested and passes existing tests.
• My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[amedina]

Nice! That's is what I am talking about!

[github-actions]

Plugin builds for 05f74ed are ready 🛎️!

• Download development build
• Download production build

[pbakaus]

OMG

[schlessera]

Nice! That's is what I am talking about!

It is noticeably (!) faster now when navigating the AMP site.

[westonruter]

For some context here, mea culpa on introducing the performance problem in #929 via 372412b#diff-ee41b32e64c90445a71328ca93cfa799R391 for the plugin's v0.7 version.

The performance issue is greatly amp-lified when Xdebug is enabled, and for that reason we added a much-maligned admin notice (#3198) which we moved a Site Health test (#4432). I still think there is value for having that Site Health test when WP_DEBUG is enabled (as we do now) since running Xdebug in production will greatly increase TTFB. Also, even with the improvement here, the style sanitizer is still slow when it has to fire up the php-css-parser when there is no cached parsed CSS cached.

[westonruter]

Cherry-picked onto 1.5 branch: e97a088