WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#47871 closed enhancement (fixed)

Cache item schema in REST controllers

Reported by: joehoyle Owned by: kadamwhite
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:

Description

The get_item_schema method on the REST controllers should be idempotent. These methods are called many times when validating schemas, sanitizing, filter_response_by_context, etc, and generating them isn't particularly cheap.

https://core.trac.wordpress.org/ticket/41305 demonstrates one such place that adds overhead, also the Posts controller get_item_schema makes many calls and asks for a lot of data: https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L1872

We should look into caching these on the class instance. Doing a count on how many times these methods are called, say on a /wp-json/ request should be easy, and indicate if there will be significant performance gains by doing this.

Attachments (1)

47871.1.diff (16.6 KB) - added by kadamwhite 2 years ago.
Add caching to default schema properties for all REST controllers

Download all attachments as: .zip

Change History (11)

#1 @kadamwhite
2 years ago

I did some basic naive performance profiling by wrapping the post controller's get_item_schema method with a microtime diff and counting how often it was called. On a wp/v2/posts collection with two posts, get_item_schema is called on the posts controller 56 times total, accounting for roughly 30ms of time.

On a collection of 100 posts, however, a wp/v2/posts?per_page=100 request generated 644 individual get_item_schema requests. Using the naive microtime logging, these calls accounted for around 290-300ms within one request's lifecycle over a series of tests, with a few outlier runs yielding up to 700ms of total time.

I would be quite keen to get this in and see those numbers drop :)

#2 @kadamwhite
2 years ago

Following up to the above, I ran a few more post counts then dumped my tests into a spreadsheet and confirmed that the time taken up by calling get_item_schema grows linearly the more posts are requested.

#3 @kadamwhite
2 years ago

  • Owner set to kadamwhite
  • Status changed from new to accepted

@kadamwhite
2 years ago

Add caching to default schema properties for all REST controllers

#4 @kadamwhite
2 years ago

  • Keywords has-patch needs-testing dev-feedback added

As a baseline on my core install, a 100-post collection was taking 964ms on average to load. With the 47871.1 patch, it is now taking 460ms. This patch could use some additional opinions, and further testing in a variety of contexts to bear out the potential performance gains.

#5 @kadamwhite
2 years ago

A note on the above patch: I opted to cache only the value of the schema computed within the get_item_schema method, and not the final output of $this->add_additional_fields_schema(). In theory we could probably cache that final value safely in most cases, as an individual REST response lifecycle is fairly self-contained and linear; but in practice the order of operations we test for in our unit test suite currently validates that we can append rest fields to an endpoint between endpoint instantiation and the delivery of the final request, and it seemed unsafe to break that assumption.

This ticket was mentioned in Slack in #core by kadamwhite. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


2 years ago

#8 @kadamwhite
2 years ago

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

In 45811:

REST API: Cache results of get_item_schema on controller instances for performance.

Caches the output of get_item_schema() to avoid redundant recomputation of translatable strings and other computed values. This method is called many times per item in each REST request, and the results of the method should not vary between calls.
Additional schema fields are not cached.

Props kadamwhite, joehoyle, TimothyBlynJacobs.
Fixes #47871.

This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.


2 years ago

#10 @johnjamesjacoby
2 years ago

My Spidey sense tingled when I saw this code comment:

// Do not cache this schema because all properties are derived from parent controller.

Curious if this change will impact anyone's expectations of how this meta data is being refreshed.

Note: See TracTickets for help on using tickets.