Make WordPress Core

Opened 6 years ago

Last modified 8 months ago

#34564 new defect (bug)

WP_List_Table::get_column_info() inefficient

Reported by: bobbingwide Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.3
Component: Administration Keywords: needs-unit-tests has-patch
Focuses: performance Cc:


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 (9)

#1 @bobbingwide
6 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.

11 months ago

#3 @mikeschroder
11 months 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
11 months 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.

11 months ago

  • Keywords has-patch added; needs-patch removed

#6 @peterwilsoncc
11 months 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
11 months 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.

8 months ago

#9 @chaion07
8 months 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] With Beta 1 coming up in a day this could be a good candidate for future Releases. Milestone updated. Thanks

Note: See TracTickets for help on using tickets.