Make WordPress Core

Opened 9 years ago

Closed 3 years ago

Last modified 2 years ago

#34564 closed defect (bug) (fixed)

WP_List_Table::get_column_info() inefficient

Reported by: bobbingwide's profile bobbingwide Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version: 4.3
Component: Administration Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: performance Cc:

Description

Changes in 4.3 to add a default column have left the logic for get_column_info() very inefficent with get_primary_column_name() being called for every single row being displayed.

In my list table displaying 50 rows the list_table_primary_column filter was invoked 52 times. My get_columns() method was called 105 times.
and that meant that the gettext filter was invoked 105 times for each of the defined columns.

That's an awful lot of work for a field which is supposed to have been cached.

Here's the relevant part of the backtrace

0. bw_lazy_backtrace C:\apache\htdocs\wordpress\wp-content\plugins\oik\libs\bwtrace.php:108 0
1. bw_backtrace C:\apache\htdocs\wordpress\wp-content\plugins\schunter\admin\class-schunter-list-table.php:74 0
2. get_columns C:\apache\htdocs\wordpress\wp-content\plugins\oik\admin\class-bw-list-table.php:871 0
3. get_default_primary_column_name C:\apache\htdocs\wordpress\wp-content\plugins\oik\admin\class-bw-list-table.php:898 0
4. get_primary_column_name C:\apache\htdocs\wordpress\wp-content\plugins\oik\admin\class-bw-list-table.php:936 0
5. get_column_info C:\apache\htdocs\wordpress\wp-content\plugins\oik\admin\class-bw-list-table.php:1007 0
6. print_column_headers() C:\apache\htdocs\wordpress\wp-content\plugins\oik\admin\class-bw-list-table.php:1099 1
7. display C:\apache\htdocs\wordpress\wp-content\plugins\schunter\admin\schunter.php:78 0 

Note: class-bw_list-table.php is basically a copy of class-list-table.php

The code in question is the new logic that doesn't trust $_column_headers, introduced in #25408.

Change History (19)

#1 @bobbingwide
9 years ago

I don't know what was wrong with the logic that checked the number of entries in $this->_column_headers.

Also, as a passing comment, I have no idea what 'horse reasons' is meant to mean.

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


4 years ago

#3 @kirasong
4 years ago

  • Focuses performance added
  • Keywords needs-patch needs-unit-tests added
  • Milestone set to 5.8

Hi @bobbingwide!

Sorry it's taken so long for a response.
This was talked about in today's triage.

It looks like "horse reasons" basically means "arbitrary reasons" or "unknown reasons" in this particular case, given the commit message. As this was also confusing to some other folks in the chat, that'd probably make sense to change.

@peterwilsoncc suggested that a next step would be looking into whether get_primary_column_name() can be cached, either as a property of the class or as a static variable in the function, and suggested 5.8 as a milestone for consideration.

#4 @peterwilsoncc
4 years ago

In 50516:

Docs: Improve compatibility comment in WP_List_Table.

Improve comment explaining backward compatibility check for the primary column in WP_List_Table::get_column_info().

Unprops helen, markjaquith.
See #52628, #34564.

This ticket was mentioned in PR #1085 on WordPress/wordpress-develop by peterwilsoncc.


4 years ago
#5

  • Keywords has-patch added; needs-patch removed

#6 @peterwilsoncc
4 years ago

In the linked pull request:

  • only run backward compatibility code if column headers are set incorrectly
  • cache backward compatibility fix
  • return cached data immediately if the column headers property contains four items

Consider: changing count() === 4 condition to check for four or more items to account for plugins adding additional items to the cached column headers. I really hope this isn't the case.

#7 @peterwilsoncc
4 years ago

In 50517:

Docs: Improve spelling in [50516].

Props jeremyfelt.
See #52628, #34564.

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


4 years ago

#9 @chaion07
4 years ago

  • Milestone changed from 5.8 to Future Release

Thanks to @bobbingwide for reporting this. We recently reviewed this during a recent [bug-scrub session]https://wordpress.slack.com/archives/C02RQBWTW/p1623098170371300. With Beta 1 coming up in a day this could be a good candidate for future Releases. Milestone updated. Thanks

#10 @mukesh27
3 years ago

  • Keywords needs-refresh added

Hi there!

The PR need to update against trunk.

@peterwilsoncc Is this ticket is in your to-do list for upcoming release?

#11 @peterwilsoncc
3 years ago

  • Milestone changed from Future Release to 6.1

I've moved this on to the 6.1 milestone, updated the PR and requested a review from a member of the performance team.

#12 @mxbclang
3 years ago

@mukesh27 will work on updating with trunk.

#13 @mukesh27
3 years ago

  • Keywords needs-refresh removed

@peterwilsoncc PR looks great.

Do we need a unit test for these changes?

Version 0, edited 3 years ago by mukesh27 (next)

#14 @costdev
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

PR 1085 now includes PHPUnit tests.

Last edited 3 years ago by costdev (previous) (diff)

#15 @peterwilsoncc
3 years ago

  • Keywords commit added
  • Owner set to peterwilsoncc
  • Status changed from new to assigned

Tests are wrapped up, marking ready for commit.

In addition to those active on this ticket, props are due to @spacedmonkey for a code review on the PR.

#16 @peterwilsoncc
3 years ago

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

In 53868:

Administration: Improve performance of List Tables.

Improve the performance of WP_List_Table::get_column_info() by adding the primary column to the cached header values. This reduces the number of calls to the WP_List_Table::get_primary_column_name() method to once per table in line with the other header getter functions.

Props bobbingwide, chaion07, costdev, mikeschroder, mukesh27, peterwilsoncc, shetheliving, spacedmonkey.
Fixes #34564.

#18 @milana_cap
3 years ago

  • Keywords needs-dev-note add-to-field-guide added

#19 @milana_cap
2 years ago

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