WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 11 months ago

#17201 reopened enhancement

dynamic_sidebar performance

Reported by: mrubiolvn Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.1
Component: Widgets Keywords: has-patch
Focuses: Cc:

Description

I've got a few dynamic sidebars (say 6 or 7) and the dynamic_sidebar function spends 1/4 of a second only calling sanitize_title.

See the piece of code on wp-includes/widgets.php:

	if ( is_int($index) ) {
		$index = "sidebar-$index";
	} else {
		$index = sanitize_title($index);
		foreach ( (array) $wp_registered_sidebars as $key => $value ) {
			if ( sanitize_title($value['name']) == $index ) {
				$index = $key;
				break;
			}
		}
	}

That's occurs evenf if you provide an id, and not the sidebar name.
We could avoid that by checking before trying to use the sidebar name if a sidebar exists with that id.

Like so...

	if ( is_int($index) ) {
		$index = "sidebar-$index";
	} elseif ( empty($wp_registered_sidebars[$index]) || !array_key_exists($index, $sidebars_widgets) || !is_array($sidebars_widgets[$index]) || empty($sidebars_widgets[$index]) ) {
		$index = sanitize_title($index);
		foreach ( (array) $wp_registered_sidebars as $key => $value ) {
			if ( sanitize_title($value['name']) == $index ) {
				$index = $key;
				break;
			}
		}
	}

Attachments (1)

revision.diff (1.0 KB) - added by mrubiolvn 3 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 mrubiolvn3 years ago

Testing the code seems that

$sidebars_widgets = wp_get_sidebars_widgets();

need to be moved on top of the function also.

mrubiolvn3 years ago

comment:2 mrubiolvn3 years ago

  • Keywords has-patch added

comment:3 mrubiolvn3 years ago

  • Keywords dev-feedback added
  • Summary changed from sanitize_title on dynamic_sidebar: performance to dynamic_sidebar performance

I'd like this fixed, don't know what else you need. Maybe a fancier ticket title... there it goes :) Thanks.

comment:4 c3mdigital11 months ago

  • Keywords close added; has-patch dev-feedback removed
  • Resolution set to invalid
  • Status changed from new to closed

I don't see a performance issue here and having 7 dynamic sidebars on a page seems like an edge case to me.

comment:5 helen11 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

sanitize_title() is pretty slow, actually. What would switching to something like sanitize_key() do? I don't really know the history behind why it's the way it is, but it seems to me at a glance that it might not be much of a breaking change, if at all.

Last edited 11 months ago by helen (previous) (diff)

comment:6 mark-k11 months ago

Maybe I miss something but that code doesn't make any sense to me, why is there a sanitation at all?

The theme developer most likely use the same sidebar name in the call to register_sidebar and dynamic_sidebar, but only one of them is sanitized? this looks like a bug to me.

comment:7 helen11 months ago

No idea what the history is behind that. Seems like maybe it came directly from the plugin that was absorbed; possibly at one point it only supported numeric IDs and this was to deal with changing to names. In my eyes, it doesn't seem necessary to use something so expensive, especially given that registering a sidebar with one name and then trying to display it with another version of that name seems like a weird thing to do.

comment:8 brianvan11 months ago

Not sure if it's relevant but ticket #5326 addressed sanitize_title() on widget IDs and recommended purging use of the function from newer API constructs because of its poor performance
http://core.trac.wordpress.org/ticket/5326

A quick look at sanitize_title() shows that it's a small function that includes an apply_filters() call that we may want to unwind to see how many expensive functions are being called for it. It also calls remove_accents(), and that function includes almost 800 calls to chr() plus a few calls to strtr() that may or may not be performance bottlenecks. Some sites say strtr() is performant, but then there's this: http://www.simplemachines.org/community/index.php?topic=175031.0

Another performance observation: dynamic_sidebar() calls the very expensive sanitize_title() function simply to compare two previously-unsanitized titles for a match. For this purpose, is the sanitization needed during iteration, or should it just be done right after foreach()? That would save a majority of the calls to sanitize_title().

Or if that's not acceptable, I wonder if any part of register_sidebar() (including actions attached within) are already calling sanitize_title() on widget titles. Maybe that data can be persisted in the widget object/array to avoid repeat sanitization calls later? Even just inserting a new sanitize_title() call once for every sidebar during registration is almost always more efficient than calling it on an average of half the array for every display_sidebar() call, with few exceptions.

comment:9 mark-k11 months ago

I looked at all the other places the sidebar name is used and in none of them it is sanitized, only escaped when displayed. maybe the sanitization is an echo for the days before the use of escaping? in any case I think it can be safely removed, and since it is not done in any other place, there is no point in caching it.

comment:12 SergeyBiryukov11 months ago

  • Keywords has-patch added; close removed
Note: See TracTickets for help on using tickets.