Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#20838 closed enhancement (fixed)

Don't populate $col_info in production

Reported by: grahamkelly's profile GrahamKelly Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.3.2
Component: Database Keywords: has-patch
Focuses: Cc:

Description

While profiling WordPress 3.3.2 I noticed that mysql_fetch_field() seemed to be fairly expensive. Upon looking into this I found that the col_info property is being set in wpdb but it does not appear (from some grepping through WP Core and quite a few extensions) that anyone is actually making use of the results (Either from directly accessing $col_info or from calls to get_col_info). In order to improve performance I wrapped the calls in 'if (WP_DEBUG)' which seems to do the trick.

Patch attached.

Attachments (3)

wpdb.patch (697 bytes) - added by GrahamKelly 12 years ago.
lazy-col-info.diff (2.1 KB) - added by pento 12 years ago.
lazy-col-info.2.diff (2.2 KB) - added by pento 12 years ago.

Download all attachments as: .zip

Change History (16)

@GrahamKelly
12 years ago

#1 @johnbillion
12 years ago

Out of curiosity, is your profiling data available? While it probably is a good idea to only fetch this while in debug mode (or maybe while SAVEQUERIES is true) I have previously profiled mysql_fetch_field and found the performance impact is negligable.

#2 @nacin
12 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to 3.5

This could be an easy lazy-loading win.

We could use a magic getter to handle someone accessing col_info directly, then make get_col_info() act on $this->result directly. This would prevent mysql_fetch_field() from firing unless someone needs it.

#3 @markjaquith
12 years ago

GrahamKelly — can you please share your profiling data?

@pento
12 years ago

#4 @pento
12 years ago

  • Keywords has-patch added; needs-patch removed

New patch uses a magic getter to only load column info when requested.

The only downside of this is that we can't do the mysql_free_result( $this->result ) in wpdb::query(), so I've moved it to wpdb::flush().

#5 @pento
12 years ago

Patch 2 changes the access visibility of wpdb::col_info and wpdb::get_col_info() to protected - they may need to be over-ridden by replacements for wpdb.

#6 @nacin
12 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [21472]:

Lazy-load column info in wpdb. props pento. fixes #20838.

#7 follow-up: @josephscott
12 years ago

  • Cc joseph@… added

The committed code - r21472 - includes error suppression:

@mysql_free_result( $this->result );

The PHP docs for mysql_free_result indicate that this is a problem only when a non-resource is provided:

If a non-resource is used for the result, an error of level E_WARNING will be emitted. It's worth noting that mysql_query() only returns a resource for SELECT, SHOW, EXPLAIN, and DESCRIBE queries.

Can we get a way with not having the @ with something like:

if ( is_resource( $this->result ) ) {
    mysql_free_result( $this->result );
}

?

It would be worth while to avoid the use of @ when ever possible.

#8 in reply to: ↑ 7 ; follow-up: @nacin
12 years ago

Replying to josephscott:

It would be worth while to avoid the use of @ when ever possible.

We discussed this as well. I agree this is a worthwhile change. New ticket?

#9 in reply to: ↑ 8 @SergeyBiryukov
12 years ago

Replying to nacin:

We discussed this as well. I agree this is a worthwhile change. New ticket?

#21533

#10 @nacin
12 years ago

In [21511]:

Eliminate error suppression for mysql_free_result() and only call it when the result is actually a resource. Depending on the query, mysql_query() can return a boolean rather than a resource, hence the original use of error suppression.

Fixes a warning introduced in [21472] when calling mysql_free_result() was moved to flush().

fixes #20838.

#11 @nacin
12 years ago

[21511] also fixes #21533.

#12 follow-up: @jeffmcmahan
11 years ago

Could someone update the $wpdb documentation at Codex to reflect this change? I don't think I'm competent to do so. But I can see that get_col isn't a lot of use anymore, is it?

#13 in reply to: ↑ 12 @nacin
11 years ago

Replying to jeffmcmahan:

Could someone update the $wpdb documentation at Codex to reflect this change? I don't think I'm competent to do so. But I can see that get_col isn't a lot of use anymore, is it?

There's no public API change here. We simply now load column information on demand, rather than with each query.

Note: See TracTickets for help on using tickets.