Make WordPress Core

Opened 22 months ago

Closed 6 months ago

Last modified 4 months ago

#52992 closed enhancement (fixed)

REST API: Avoid unnecessarily preparing item links

Reported by: dlh's profile dlh Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.1 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests needs-testing needs-testing-info commit has-dev-note
Focuses: performance Cc:

Description

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.
  1. 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.

Change History (29)

This ticket was mentioned in PR #1179 on WordPress/wordpress-develop by dlh01.


22 months ago
#1

  • Keywords has-unit-tests added

#2 @rachelbaker
22 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 5.8
  • Owner set to rachelbaker
  • Status changed from new to accepted

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.

#3 @desrosj
20 months ago

  • Milestone changed from 5.8 to 5.9

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

Punting to 5.9 as there has been good recent momentum.

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


15 months ago

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


15 months ago

#6 @hellofromTonya
15 months ago

  • Keywords needs-refresh needs-testing-info added

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.

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


15 months ago

#8 @hellofromTonya
15 months ago

  • Milestone changed from 5.9 to Future Release

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.

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


15 months ago

#10 @dlh
15 months ago

  • Keywords needs-refresh removed

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.

#11 @johnbillion
15 months ago

  • Focuses performance added

#12 @johnbillion
8 months ago

  • Milestone changed from Future Release to 6.0

#13 @johnbillion
8 months ago

  • Milestone changed from 6.0 to 6.1

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


8 months ago

#16 @spacedmonkey
8 months ago

  • Keywords needs-refresh added

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 ?

#17 @dlh
8 months ago

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

#18 @spacedmonkey
7 months ago

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.

dlh01 commented on PR #1179:


7 months ago
#19

Will open a fresh PR against trunk.

This ticket was mentioned in PR #2884 on WordPress/wordpress-develop by dlh01.


7 months ago
#20

  • Keywords needs-refresh removed

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.

#21 @dlh
7 months ago

@spacedmonkey A few updates:

  • 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%  |
+-----------------------+-------------------+-----+------+-----+-----------------+-----------------+----------------+

#23 @spacedmonkey
6 months ago

  • Keywords commit added
  • Owner changed from rachelbaker to spacedmonkey
  • Status changed from accepted to assigned

#24 @spacedmonkey
6 months ago

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

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.

spacedmonkey commented on PR #3009:


6 months ago
#25

Committed.

spacedmonkey commented on PR #2884:


6 months ago
#26

Committed.

#27 @spacedmonkey
4 months ago

  • Keywords needs-dev-note added

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


4 months ago

Note: See TracTickets for help on using tickets.