REST API: Avoid unnecessarily preparing item links

[dlh]

The bundled REST API controllers currently prepare links for each item in a REST response regardless of whether the links will appear in the response or are needed to fulfill an _embed request.

For example, when responding to the request from the block editor for available hierarchical terms:

?per_page=100&orderby=name&order=asc&_fields=id,name,parent&_locale=user

WP_REST_Terms_Controller will call prepare_links() for all found terms even though the _links property is omitted from each, adding unnecessary processing that delays the response to the client.

Based on ​a discussion in the core Slack, the change proposed for this ticket would:

1. Include _links by default in WP_REST_Controller::get_fields_for_response(), and include _embedded if the request object includes the _embed parameter. As discussed in Slack, these fields are included as a convention. The fields aren't typically included in the item schema, and because prepare_links() isn't a method defined on the base controller, it isn't possible for the controller to know before the response is prepared whether it will actually include links.

2. Wrap the calls to prepare_links() and add_links() in each of the bundled controllers in checks for whether the _links field or _embedded field is included in the response.

Lastly, during the Slack discussion, Timothy asked:

Do you think we should bother checking for each individual link? We could theoretically do rest_is_field_included( '_links.self' ), etc...

I've opted to not do this in the current patch, simply to avoid the complexity of modifying the prepare_links() signatures across the various controllers to accept the requested fields or the request object, but I'm happy to do so if desired.

Trac ticket: https://core.trac.wordpress.org/ticket/52992

[rachelbaker]

Thanks for putting together the patch @dlh. I added needs testing to ensure this is more of a performance change versus a breaking API change.

[desrosj]

Today is 5.8 feature freeze. Unfortunately this one ran out of time.

Punting to 5.9 as there has been good recent momentum.

[hellofromTonya]

Marking as needs-refresh as there are merge conflicts with the PR.

In review with the test team, is manual behavior testing needed? If yes, then step-by-step instructions are needed to perform these manual tests and provide test reports. Marking as needs-testing-info. If performance benchmarking testing is required and not manual behavior or smoke testing, please add a comment and remove this keyword. Thanks.

Please note, 5.9 feature freeze quickly approaching, i.e. Nov 9th. There's potentially still time for this ticket.

[hellofromTonya]

In REST meeting today, team agreed to punt. Why? There are unknowns that need soak time and consideration. Concerned there's not enough time left before 5.9 feature freeze.

Moving to Future Release as the next release cycle is not available yet.

However, if these concerns are addressed and it's ready for commit before feature freeze, please feel free to move back into the 5.9 milestone.

[dlh]

I've updated the Pull Request to address the merge conflicts.

I've also added a PHPBench benchmarking class that evaluates the performance of dispatching the same REST API request to the terms endpoint that was included in the ticket description.

(The class was modeled on a benchmarking class in a Pull Request for #53218: ​https://github.com/WordPress/wordpress-develop/pull/1259.)

The benchmarking class is not meant to be committed, of course, but it should allow for others to run the same benchmarks for the changes in this ticket or explore their own. I also hadn't used PHPBench before, so it would be good to have more pairs of eyes on the benchmark class itself for any mistakes.

To add PHPBench to my local development environment for testing, I ran composer require phpbench/phpbench --dev from the wordpress-develop root.

I then used ​steps from the PHPBench documentation to create a baseline measurement on the master branch:

vendor/bin/phpbench run tests/Benchmark/ --tag=original

and then a comparison with the branch in the Pull Request:

vendor/bin/phpbench run tests/Benchmark/ --report=aggregate --ref=original

In my local environment, running PHP 7.4, PHPBench typically reported around a 60% decrease in wall time and a minor decrease in memory usage to dispatch the request with the patch applied. Here's an example report from the comparison run:

+-----------------------+-------------------+-----+------+-----+-----------------+-----------------+-----------------+
| benchmark             | subject           | set | revs | its | mem_peak        | mode            | rstdev          |
+-----------------------+-------------------+-----+------+-----+-----------------+-----------------+-----------------+
| RestPrepareLinksBench | bench_default     |     | 1000 | 5   | 23.624mb -1.72% | 2.555ms -64.15% | ±1.23% -61.63%  |
| RestPrepareLinksBench | bench_with_filter |     | 1000 | 5   | 23.625mb -1.71% | 2.479ms -67.04% | ±3.42% +269.48% |
+-----------------------+-------------------+-----+------+-----+-----------------+-----------------+-----------------+

In terms of manual or smoke testing, the main things to check would be:

1. The _links property is included in REST API responses when _fields isn't specified or when it includes _links.
2. Requests with _embed included the expected embedded resources.

Missed links.

​https://github.com/WordPress/wordpress-develop/blob/df8c3d0e089f77eb5c6fa971ba4ae9956255893e/src/wp-includes/rest-api/endpoints/class-wp-rest-sidebars-controller.php#L371

​https://github.com/WordPress/wordpress-develop/blob/bd08b221273b8b367a18fef8c9efb873819ac89d/src/wp-includes/rest-api/endpoints/class-wp-rest-templates-controller.php#L665

​https://github.com/WordPress/wordpress-develop/blob/78cf2760354fe508d4abaf19403514034cbef1c9/src/wp-includes/rest-api/endpoints/class-wp-rest-widget-types-controller.php#L338

​https://github.com/WordPress/wordpress-develop/blob/df8c3d0e089f77eb5c6fa971ba4ae9956255893e/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php#L729

[spacedmonkey]

I have spent some time reviewing this patch. It seems simple enough and a solid change. The patch needs a refresh.

I think this can make it into 6.1. Are you still happy to own and commit - @rachelbaker ?

[dlh]

I have some time set aside next week to refresh the patch.

[spacedmonkey]

For reference, there are been some performance improvements to the REST API. See #55716, #55674, #55677, #55592, #55593. A patch should be tested against trunk.

Will open a fresh PR against trunk.

Trac ticket: https://core.trac.wordpress.org/ticket/52992

Previously #1179. Includes the changes from #2883, which were suggested in the old PR but have since been split into different tickets.

[dlh]

@spacedmonkey A few updates:

• I've opened ​https://github.com/WordPress/wordpress-develop/pull/2883 with changes that you had originally suggested in the context of this ticket and that are now covered by tickets #56019 and #56020.

• I closed my previous PR for this ticket and opened a new one with a refreshed patch: ​https://github.com/WordPress/wordpress-develop/pull/2884. The second PR is based on the first, so it includes the same changes.

• Regarding performance, the PHPBench benchmarking class that I used before now reports around a 50% decrease in wall time with the patch applied, compared to a 60% decrease when this ticket was reviewed last fall.

+-----------------------+-------------------+-----+------+-----+-----------------+-----------------+----------------+
| benchmark             | subject           | set | revs | its | mem_peak        | mode            | rstdev         |
+-----------------------+-------------------+-----+------+-----+-----------------+-----------------+----------------+
| RestPrepareLinksBench | bench_default     |     | 1000 | 5   | 27.265mb -1.02% | 1.473ms -54.02% | ±3.44% -19.93% |
| RestPrepareLinksBench | bench_with_filter |     | 1000 | 5   | 27.268mb -1.02% | 1.452ms -56.60% | ±1.31% -9.83%  |
+-----------------------+-------------------+-----+------+-----+-----------------+-----------------+----------------+

Trac ticket: https://core.trac.wordpress.org/ticket/52992

[spacedmonkey]

In 53760:

REST API: Avoid unnecessarily preparing item links.

Do not call the prepare_links methods in core REST API controllers, unless the _links or _embedded fields are requested. There is no need to prepare links if they are never returned in the response. This saves resources, as many calls to prepare_links methods perform database queries.

Props Spacedmonkey, timothyblynjacobs, rachelbaker, desrosj, dlh, hellofromTonya.
Fixes #52992.

Committed.

Committed.

[milana_cap]

Dev note: ​https://make.wordpress.org/core/2022/10/10/performance-improvements-to-the-rest-api/