Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#55392 closed defect (bug) (fixed)

Performance improvements to get_user_data_from_wp_global_styles

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.1 Priority: normal
Severity: normal Version: 5.9
Component: Posts, Post Types Keywords: has-patch has-unit-tests add-to-field-guide
Focuses: performance Cc:

Description

The get_user_data_from_wp_global_styles in the WP_Theme_JSON_Resolver class have a number of performance issues. These include by are not limited to.

The following params should be passing the WP_Query object to increase performance.

'ignore_sticky_posts'    => true,
'suppress_filters'       => false,
'update_post_meta_cache' => false,
'update_post_term_cache' => false,
'lazy_load_term_meta'    => false,
'no_found_rows'          => true,
'suppress_filters'       => false,
  • Cache invalidation now happens when posts are not updated and not just on an hour.
  • Handle if wp_insert_posts returns a WP_Error.
  • Only return ids in query, to make query simplifier.


Attachments (2)

Screenshot 2022-06-13 at 22.07.09.png (11.3 KB) - added by spacedmonkey 2 years ago.
Before
Screenshot 2022-06-13 at 22.07.26.png (12.2 KB) - added by spacedmonkey 2 years ago.
After

Download all attachments as: .zip

Change History (23)

spacedmonkey commented on PR #2414:


2 years ago
#2

I have added some improved unit tests and tweaked this function so it now caches even more.

CC @oandregal @Mamaduka for a review, as they have worked on this function in the past.

#3 @spacedmonkey
2 years ago

  • Keywords has-unit-tests added
  • Milestone changed from Awaiting Review to 6.1

spacedmonkey commented on PR #2414:


23 months ago
#4

I think this is great, but I wonder if this wouldn't be better as a Gutenberg PR instead of a core patch. i fear that if a gutenberg specific resolver class is added in compat merging will be hell.

@draganescu Now that this code lives in core, should PR not be done here? Would this not mean that have to port it over, adding more steps. This process, is not documented and is confusing me to.

draganescu commented on PR #2414:


23 months ago
#5

This process, is not documented and is confusing me to.

This is true! But the code is still being heavily changed so I think to make Gutenberg plugin merges simpler we should not patch core with anything that is related to Gutenberg and can be created as a Gutenberg patch /PR.

#6 @adamsilverstein
21 months ago

@spacedmonkey this looks like a solid performance enhancement, bravo! Love to see these wins in theme code as well since it is so potentially impactful.

I see some failing tests under PHP 7.4 on the PR - is there anything else needed before this can land?

#7 @spacedmonkey
21 months ago

I have spent a little time on this. This change is still worth merging, as it skipping all the logic in WP_Query takes away some over head.

I will look into why tests are failing and merge.

#8 @spacedmonkey
20 months ago

@adamsilverstein Can you review again. Tests are now passing.

#9 @mukesh27
20 months ago

@spacedmonkey look solid, I left some feedback on the PR.

#10 @spacedmonkey
20 months ago

  • Owner set to spacedmonkey
  • Resolution set to fixed
  • Status changed from new to closed

In 54186:

Posts, Post Types: Improve performance of the get_user_data_from_wp_global_styles method.

Improve the logic found in get_user_data_from_wp_global_styles method. Replace call to wp_get_recent_posts with the more standard, WP_Query for consistancy. Use transient over standard cache, to improve performance on sites without persistent object caching. Improve handling of cases where wp_insert_post returns a WP_Error.

Props spacedmonkey, adamsilverstein, mukesh27, peterwilsoncc, andregal.
Fixes #55392.

spacedmonkey commented on PR #2414:


20 months ago
#11

Committed into core.

#12 @kebbet
20 months ago

  • Keywords needs-refresh added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Latest changeset goes against #54518, see [52280]

Last edited 20 months ago by kebbet (previous) (diff)

This ticket was mentioned in PR #3263 on WordPress/wordpress-develop by kebbet.


20 months ago
#13

  • Keywords needs-refresh removed

mukeshpanchal27 commented on PR #3263:


20 months ago
#14

Thanks for flagging the issue.

In core we use similar core reference tickets. There is no specific format for this type of document.

https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-menu-items-controller.php#L331
https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-menu-items-controller.php#L333
https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/blocks/gallery.php#L84

Let's update the message and use period at end.

// Do not make string translatable, see https://core.trac.wordpress.org/ticket/54518.

kebbet commented on PR #3263:


20 months ago
#15

Thanks for flagging the issue.

In core we use similar core reference tickets. There is no specific format for this type of document.

https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-menu-items-controller.php#L331 https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-menu-items-controller.php#L333 https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/blocks/gallery.php#L84

Let's update the message and use period at end.

// Do not make string translatable, see https://core.trac.wordpress.org/ticket/54518.

Thanks for your review @mukeshpanchal27. The two commits adds a preiod and removes ticket from the comment.

#16 @mukesh27
20 months ago

The PR looks good to me and ready for the commit.

@spacedmonkey Can you please review it?

#17 @kebbet
20 months ago

@spacedmonkey I can not see the reason why the post_title was made translatable in [54186]. Could you please review the PR that reverts that particular change?

#18 @spacedmonkey
20 months ago

In 54246:

Posts, Post Types: Post title should not be translatable in get_user_data_from_wp_global_styles method.

In [52280] wp_template_part and wp_template posts, made the title of the post created not translatable. This was changed [54186] by mistake. This commit reverts that change.

Follow up to [54186], [52280].

Props mukesh27, kebbet.
See #55392.

#19 @spacedmonkey
20 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

@kebbet @mukesh27

Thanks for the flag. Fixed in [54246].

#21 @milana_cap
20 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.