#34564 closed defect (bug) (fixed)
WP_List_Table::get_column_info() inefficient
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.
4 years ago
#3
@
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.
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
@
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.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
4 years ago
#9
@
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
@
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
@
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.
#13
@
3 years ago
- Keywords needs-refresh removed
@peterwilsoncc PR looks great.
Do we need a unit test for these changes?
#14
@
3 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
PR 1085 now includes PHPUnit tests.
#15
@
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.
peterwilsoncc commented on PR #1085:
3 years ago
#17
Merged https://core.trac.wordpress.org/changeset/53868 / 11cc06cd22
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.