dynamic_sidebar performance

[mrubiolvn]

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;
			}
		}
	}

[mrubiolvn]

Testing the code seems that

$sidebars_widgets = wp_get_sidebars_widgets();

need to be moved on top of the function also.

[mrubiolvn]

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

[c3mdigital]

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

[helen]

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.

[mark-k]

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.

[helen]

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.

[brianvan]

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.

[mark-k]

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.

[SergeyBiryukov]

Related: #22116, #23423

[SergeyBiryukov]

Related: [5473] (for #4258).