New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validator performance with bulk publishing posts #1404

Closed
ricardobrg opened this Issue Sep 6, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@ricardobrg
Contributor

ricardobrg commented Sep 6, 2018

When creating lots of posts through a importing script, the server CPU usage gets really high because it runs the validation for all posts at once.

I've seen this happen with WordPress Importer and WooCommerce import. Althought the WooCommerce support is not available yet, this issue has to do with any kind of bulk publishing posts.

I didn't check the source yet but maybe we could use a flag, or even better, a queue for validator hooked function, to prevent it to be executed for lots of posts at once. And it could be a simple variable set in the AMP configuration a sort of "limit of simultaneous validator instances running".

@westonruter westonruter added this to the v1.0 milestone Sep 6, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 6, 2018

It's very important that you have brought this up! We should short-circuit the validation request when WP_IMPORTING constant is defined. The WordPress importer also does wp_suspend_cache_invalidation( true ) to speed up the import process, and we could potentially short-circuit when wp_suspend_cache_invalidation() returns true.

The code in question is this handle_save_post_prompting_validation method which runs at the save_post action:

https://github.com/Automattic/amp-wp/blob/db6044260aa3ed25326b0e0017ba8a933644d3a8/includes/validation/class-amp-validation-manager.php#L467-L496

You can see there are several checks for confirming whether or not to proceed.

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 6, 2018

@ricardobrg Does WooCommerce also define WP_IMPORTING when importing products?

@ricardobrg

This comment has been minimized.

Contributor

ricardobrg commented Sep 6, 2018

@westonruter looks like it doesn't. I will open an issue there too, asking to implement it.

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 6, 2018

We would also want to prevent validation when doing bulk creation of posts via WP-CLI, for example.

@ricardobrg

This comment has been minimized.

Contributor

ricardobrg commented Sep 12, 2018

@westonruter I think that the best approach would be working with a queue for validation. This way we don't depend on how the other plugins/themes/cli/core/etc are publishing and improve the compatibility of the plugin. We just limit simultaneous validations and queue the rest.

Looks like somebody already thought about it:

https://github.com/Automattic/amp-wp/blob/db6044260aa3ed25326b0e0017ba8a933644d3a8/includes/validation/class-amp-validation-manager.php#L515

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 12, 2018

That was me 😄

da5f4ad#diff-7d234989187d87a372e2aa6aa1299a71R330

I think actually that we should indeed only check the first post for which a save_post is triggered. In the case of an actual bulk action to re-validate, this is not using the save_post action so it wouldn't be impacted:

https://github.com/Automattic/amp-wp/blob/b3e1b4d07dda70c5070d30f7426f849eb3a39823/includes/validation/class-amp-invalid-url-post-type.php#L700-L711

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 12, 2018

@ricardobrg please give #1424 a test.

@ricardobrg

This comment has been minimized.

Contributor

ricardobrg commented Sep 13, 2018

#1424 fixed it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment