Make WordPress Core

Opened 10 years ago

Closed 9 years ago

Last modified 8 years ago

#32103 closed defect (bug) (fixed)

Multidimensional Customizer settings (options & theme mods) are not scalable

Reported by: air's profile Air. Owned by: westonruter's profile westonruter
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.4
Component: Customize Keywords: has-patch commit
Focuses: performance Cc:

Description

When you have theme settings saved as serialized data(like explained in ottos tutorial in point 3 of paragraph "Existing Options" http://ottopress.com/2012/how-to-leverage-the-theme-customizer-in-your-own-themes/ ), customizer will sanitize data as many times, as many options there are in this array of settings. Per option! So if you have like 20 options in such setting, and you change 1, sanitize callback will be called 20 times, for this one setting. If I change 3 options, then it will be called 60 times, 57 too much:-) And I have big array of settings, like 60 per section, and also heavy sanitize function so it gives big waste of time.

Now to be more detailed: if we have Array with three settings like this:
nice_theme_settings[setting1]
nice_theme_settings[setting2]
nice_theme_settings[setting3]
....
nice_theme_settings[setting20]

and we use "sanitize_callback" then for each such option is registered filter in preview method(file wp-includes\class-wp-customize-setting.php ~155:

	public function preview() {
		...

		switch( $this->type ) {
			....
			case 'option' :
				if ( empty( $this->id_data[ 'keys' ] ) )
					add_filter( 'pre_option_' . $this->id_data[ 'base' ], array( $this, '_preview_filter' ) );
				else {
					add_filter( 'option_' . $this->id_data[ 'base' ], array( $this, '_preview_filter' ) );
					add_filter( 'default_option_' . $this->id_data[ 'base' ], array( $this, '_preview_filter' ) );
				}
				break;
			....

		
		}
	}

All these filters that add filters using " $this->id_data[ 'base' ]" will add X(20 in this case) times filter that looks like this:

add_filter( 'option_nice_theme_settings', 'function' );

In other place wordpress uses id as variable part of filter so it looks proper, like this:

add_filter( 'option_nice_theme_settings[setting3]', 'function' );

Attachments (5)

32103.wip.diff (18.1 KB) - added by westonruter 9 years ago.
WIP
32103.diff (12.1 KB) - added by westonruter 9 years ago.
https://github.com/xwp/wordpress-develop/pull/124
32103.wip2.diff (30.7 KB) - added by westonruter 9 years ago.
32103.wip3.diff (30.5 KB) - added by westonruter 9 years ago.
Fixed unit tests
32103.4.diff (28.5 KB) - added by westonruter 9 years ago.
https://github.com/xwp/wordpress-develop/pull/125

Download all attachments as: .zip

Change History (51)

#1 @SergeyBiryukov
10 years ago

  • Summary changed from Customizer snitize data multiple times when options are served as Serialized Settings to Customizer sanitizes data multiple times when options are served as Serialized Settings

#2 @Air.
10 years ago

  • Focuses performance added

#3 @westonruter
10 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to westonruter
  • Status changed from new to accepted
  • Version changed from 4.2 to 3.4

Yes, this is a problem I've noticed recently. It is especially problematic/evident with widgets, when you have a lot of them. I was noticing the Customizer's sanitization methods being called 50K+ times each time with each request. The workaround/solution I've put in place for a current client project (for which I'll be working on a patch to share) is to register a new Customizer setting type for widget, and then to store the unserialized settings in memory, and perform the sanitization on just the specific setting instance as needed. Then when saving the Customizer setting, the whole array gets re-serialized and stored in the option.

As an aside, widgets are not scalable as they exist in Core right now. I've been working on a feature plugin that introduces a widget_instance post type to store each widget individually as a separate DB entry (in the wp_posts table). This also gets around the poor performance of having to re-serialize the entire option value.

#4 @Air.
10 years ago

50k! Wow, I didn't expect this to go above 1k in extreme scenarios. I didn't investigate code any further, but do we even need these filters with id_data[ 'base' ] instead of id ? Id alone should do the trick in case of single option and array of options. Maybe I am missing something.

#5 @westonruter
10 years ago

  • Milestone changed from Future Release to 4.3

#6 follow-up: @Aniruddh
10 years ago

Kudos to Air.

Is it possible to target this for 4.2.2? I believe many theme developers go for serialized value structure and this seems to be a serious performance issue.

#7 in reply to: ↑ 6 @westonruter
10 years ago

Replying to Aniruddh:

Kudos to Air.

Is it possible to target this for 4.2.2? I believe many theme developers go for serialized value structure and this seems to be a serious performance issue.

I've pushed up the implementation that I've done specifically for improving the efficiency of sanitizing widget settings in the Customizer, which I believe is the overwhelmingly worst offender for performance: https://github.com/xwp/wp-customize-widgets-plus/pull/6/files

The approach here might be able to be generalized for any multidimensional setting, but I haven't thought it through.

I've submitted this Customize Widgets Plus plugin to the WordPress.org plugin directory, but for now the feature plugin can be obtained on GitHub: https://github.com/xwp/wp-customize-widgets-plus

#8 @westonruter
9 years ago

As also noted in #32769 for nav_menu_item settings, one thing that would greatly improve initial load time is to lazy-load the widget instance settings for a given widget only when the sidebar section is actually expanded. This is a bit trickier for widgets because each widget needs its own PHP-generated control.

#9 @obenland
9 years ago

  • Milestone changed from 4.3 to Future Release

No patch, maybe in a future release.

#10 @westonruter
9 years ago

  • Milestone changed from Future Release to 4.4

Performance is going to be a key focus for 4.4.

@westonruter
9 years ago

WIP

#11 @westonruter
9 years ago

I think I have a solution for improving multidimensional settings in general. I'm attaching 32103.wip.diff for a work-in-progress patch for what I'm working on: it does not yet work in its current state.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


9 years ago

#13 @westonruter
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

I've scrapped the approach I was taking in 32103.wip.diff, and I've come up with a much more concise solution that should also improve performance for previewing non-multidimensional (unserialized) settings as well.

Please see 32103.diff.

The key change here is that the WP_Customize_Setting::preview() method now checks whether there has been a value saved for that setting previously (e.g. if the option or theme mod already is set) and also if the setting has a post value associated with it (i.e. it is dirty). If the setting has already has a value set and it is not dirty, then the preview method just short-circuits and prevents adding any filters at all since there is no change to apply.

I'm seeing great performance improvements with this change. My test site has 100 widgets.

Before:

  • Preview render time: 0.5 seconds
  • Number of preview filters added: 132
  • Calls to WP_Customize_Setting::multidimensional_replace(): 20,431 (!!)

After:

  • Preview render time: 0.35 seconds (30% reduction)
  • Number of preview filters added: 13 (90% reduction)
  • Calls to WP_Customize_Setting::multidimensional_replace(): 4 (99.98% reduction!!)

Please test!

#14 @westonruter
9 years ago

  • Keywords reporter-feedback added

@Air.: Can you try the patch and see how it improves performance for you?

@valendesigns or @ericlewis Could you do a code review for me? Am I missing anything?

#15 @valendesigns
9 years ago

@westonruter I can do a code review tomorrow.

#16 follow-up: @valendesigns
9 years ago

The code looks solid and well documented. I don't see any glaring issues. What is the best way to test the functionality of the patch so I can see it in action?

#17 @westonruter
9 years ago

Probably just add a bunch of widgets of a given type, and then ensure that changes can be previewed and saved as expected. Widgets are probably the most extensively-utilized multidimensional settings, so they're a good one to check with.

#18 follow-ups: @westonruter
9 years ago

#34132 was marked as a duplicate.

#19 in reply to: ↑ 18 @wpweaver
9 years ago

Replying to westonruter:

#34132 was marked as a duplicate

I tested https://github.com/xwp/wordpress-develop/blob/trac-32103-preview-short-circuit/src/wp-includes/class-wp-customize-setting.php

I get no difference at all. Still times out at 30 seconds execution using my simple example of 600 checkbox controls shown in #34132 - if I grabbed the wrong version, I apologize. I do not use github for my own stuff, so am not 100% sure I found the correct fixes.

So, it STILL is taking forever to process the settings. I think the issue is more complex than addressed by the "fix".

I see that it is more complex than the multidimensional_replace - that is just where the timeout seems to happen.

One of the culprits may be just fetching the theme options with get_option, which is filtered through the customizer a ton of times, and unserialized each time. It looks to me as if each and every time any option is fetched, this whole sequence is done.

At any rate, as noted in my other issue, I am dead in the water trying to build a Customizer version of my theme as taking well over 20 seconds to refresh is simply not acceptable behavior for an end user.

Last edited 9 years ago by wpweaver (previous) (diff)

#20 in reply to: ↑ 16 @wpweaver
9 years ago

Replying to valendesigns:

The code looks solid and well documented. I don't see any glaring issues. What is the best way to test the functionality of the patch so I can see it in action?

The code at https://github.com/xwp/wordpress-develop/blob/trac-32103-preview-short-circuit/src/wp-includes/class-wp-customize-setting.php

does not solve my issues with 600 customizer controls. See my suggestion for testing this in #34132

#21 in reply to: ↑ 18 ; follow-up: @wpweaver
9 years ago

Replying to westonruter:

#34132 was marked as a duplicate.

Sorry to reply again, but I simply must emphasize the absolute seriousness of this issue. If you are not aware, the WordPress Theme Review theme has REQUIRED that ALL themes use the Customizer. My theme may be somewhat unique, but I recently completed a survey of themes still using the old Settings API, and there are a lot of them, and many of them have a lot of options.

As they convert to Customizer, this unacceptable SLOW preview issue will become more and more of a problem for lots of themes. It simply is not acceptable for the preview to EVER, under any circumstances, to take more that a few seconds, no matter how many settings are included in the interface. 6 or 600, it should be fast and quick.

This is now a REQUIRED feature for ALL themes, and the themes are at the heart of WP in many ways. It may happen that my theme pushes the limits on the number of options, but I must say that even with 600 options, the Customizer makes it possible that the user literally only need to know about 20 important things tp easily access all of them. When in use, the interaction is brilliant. But I am having to develop just one small section at a time. I am using postMessage for about 95% of the settings, but just the initial load of the Customizer preview is taking over 20 seconds from a very fast server when I have most of the options included. And I have quite a few more, so I suspect this will never run with the current state of things.

While PART of the issue MAY have been multiple calls to sanitize, I've done some investigation, and I believe the problem is MUCH deeper, and involves multiple generated calls to get_option, and the settings must be unserialized each time. The whole process is generating thousands and thousands of calls to the built-in functions and filters.

While this might be logically nice, it doesn't work. And since this is a require interface now, it is not okay that it works only for some themes that don't push it with the number of options. It has to work even for over a thousand options.

At this point, I suspect that the only solution that will give satisfactory performance will be to quit using the standard get_option calls and the filters, and build a cache that is refreshed only once. Perhaps saving the changed options back to the option DB can be done that way, but dynamically replacing the options really should be done once and only once.

I know machines are fast these days, but designing an exponentially increasing access time to handle these options won't work even on fast machines.

I'm a Customizer believer now, and if it worked right, the new option interface I've built would be totally fantastic. As it is, it is not useable in the least. An improvement of 30% isn't enough. It needs a 95% improvement.

Sorry to rant, but this is serious, and it needs to be moved to the highest priority possible given the requirements of the theme review team.

I will be more than happy to test things, and even provide a current copy of my theme that is taking forever to refresh if that would help.

Last edited 9 years ago by wpweaver (previous) (diff)

#22 in reply to: ↑ 21 ; follow-up: @westonruter
9 years ago

Replying to wpweaver:

At this point, I suspect that the only solution that will give satisfactory performance will be to quit using the standard get_option calls and the filters, and build a cache that is refreshed only once. Perhaps saving the changed options back to the option DB can be done that way, but dynamically replacing the options really should be done once and only once.

This is, in fact, close to the original approach I was taking in 32103.wip.diff, except it was focusing on caching the previewed values and updated values. It wasn't focused on the initial values, which seems to also be a problem as you've made VERY clear.

Having 600 settings and controls is indeed very extreme, so you shouldn't be surprised that you've encountered a scalability problem, which does indeed need to be fixed and it is my goal to fix for this current release cycle. However, it will not be fixed until WordPress 4.4 is released and so you're going to need to come up with a different solution in the meantime since your situation is so DIRE indeed.

It seems like your best bet in the interim is to register your theme's multitude of settings using a custom setting type (not theme_mod or option), or by creating a new subclass of WP_Customize_Setting, and to then handle their value, preview, and update operations yourself in an efficient way (using a cached array) which bypasses entirely the performance problems in the multidimensional routines.

#23 @wpweaver
9 years ago

There should not be a scalability issue. We are talking 600, not 6 million.

But if you haven't figured this out, here is what I'm pretty sure is the issue in preview().

public function preview() {
		if ( ! isset( $this->_original_value ) ) {
			$this->_original_value = $this->value();
		}
		if ( ! isset( $this->_previewed_blog_id ) ) {
			$this->_previewed_blog_id = get_current_blog_id();
		}
		switch( $this->type ) {
			case 'theme_mod' :
				add_filter( 'theme_mod_' . $this->id_data[ 'base' ], array( $this, '_preview_filter' ) );
				break;
			case 'option' :
				if ( empty( $this->id_data[ 'keys' ] ) )
/* PROBLEM */					add_filter( 'pre_option_' . $this->id_data[ 'base' ], array( $this, '_preview_filter' ) );
				else {
/* PROBLEM */					add_filter( 'option_' . $this->id_data[ 'base' ], array( $this, '_preview_filter' ) );
/* PROBLEM */					add_filter( 'default_option_' . $this->id_data[ 'base' ], array( $this, '_preview_filter' ) );
				}
				break;
			default :

... 

I added the /* PROBLEM */ comments. What do these statements really mean? The problem is that the array( $this, '_preview_filter') is creating a brand new filter for EACH option setting. Given that the serialized theme option setting is IDENTICAL for all, there really needs to be only a single filter.

The effect is that for each setting that is looked up (filtered), it will be run through N filters, where N is the total number of settings. So if there are 600 settings, each of those settings will be run through 600 filters, each identical to the other. So, the process is 600 * 600 operations instead of a needed 600 only. That's the problem.

So what should be a order(N) algorithm has needlessly been turned into an order(N-squared) problem.

This is, I think, an unintended side effect of a normally elegant Object Oriented solution. The creation of all those objects (600 for my example) is easy, but the long standing WP filter tool is inherently a static, traditional solution, and was never designed, really, to create 600 instances of an essentially identical filter.

I find this an interesting example case of a problem with OO. And I literally wrote the book on OO (well, a book). Just FYI, I've been in CS for 40 years, including teaching CS at the university level for almost 20 years, and am building my theme as a sort of retirement activity. So I do find this particular problem a bit of an academic exercise.

The solution, I think, would be to make the _filter_preview a static function, and check if the 'option_' . $this->id_data\['base'\] filter has been created already before adding a new one. Then there is just one filter, and one filter to call.

Last edited 9 years ago by wpweaver (previous) (diff)

#24 @wpweaver
9 years ago

Final follow up - I'm sure my diagnosis is correct because my solution completely fixes the issue. It is SO cool to see an instant update no matter how many options I used. I tested with 2000 controls, and the preview updated within a couple of seconds. (The customizer panel took a bit longer to build, but that's a one shot thing.)

I use the optional alternate type case to implement my solution as you suggested earlier, but I think the principles will apply to a more general solution. The key is to build a list of the WP_Customize_Setting objects instead of adding yet another filter, then using that list in the option filters. Here's my code just to prove the concept. I hope it helps to speed up the preview code because even the few seconds the preview is taking for even 50 options is too long. This will help everyone without making them find their own solution.

I don't think the multidimensional replace code is too slow, so you can no doubt use that logic. My replacement loop is obviously specific for the settings array used by my theme. But the concept holds true.

function weaverx_weaverx_cz( $setting ) {
	// This action is called once for each Weaver Xtreme option added to the Customizer controls.

	if (!isset( $GLOBALS['weaverx_cz_settings']))
		$GLOBALS['weaverx_cz_settings'] = array();

	$GLOBALS['weaverx_cz_settings'][] = $setting;

}

add_action('customize_preview_weaverx_cz','weaverx_weaverx_cz');

function weaverx_cz_options_filter( $original ) {

	if (empty($GLOBALS['weaverx_cz_settings']))
		return $original;

	$copy = $original;

	foreach ($GLOBALS['weaverx_cz_settings'] as $setting ) {
		$id = str_replace(array('weaverx_settings[',']'), '', $setting->id);
		$value = $setting->post_value();
		if ( $value != null)
			if ( isset($copy[$id]) && $copy[$id] != $value)
				$copy[$id] = $value;
			else
				$copy[$id] = $value;
	}
	return $copy;
}

add_filter('option_weaverx_settings', 'weaverx_cz_options_filter');
add_filter('default_option_weaverx_settings', 'weaverx_cz_options_filter');
add_filter('pre_option_weaverx_settings', 'weaverx_cz_options_filter');

#25 in reply to: ↑ 22 @wpweaver
9 years ago

Replying to westonruter:

It seems like your best bet in the interim is to register your theme's multitude of settings using a custom setting type (not theme_mod or option), or by creating a new subclass of WP_Customize_Setting, and to then handle their value, preview, and update operations yourself in an efficient way (using a cached array) which bypasses entirely the performance problems in the multidimensional routines.

The custom setting type won't work because the existing code has a bug in update that return do_action(xxx). I added a ticket for that. Has anyone actually tested and got a working custom type? Can't see how with that code there.

And to use a subclass, you have to instantiate it, right? That happens in WP_Customize_Manager, and I don't have access to that, either. So I don't see how creating a fixed WP_Customize_Setting will help anything because I can't get it instantiated anywhere that I can find.

And the multidimensional routines have nothing to do with the real problem - the problem is the order(n-squared) issue in creating the multiple filters for each setting in preview().

#26 @westonruter
9 years ago

@wpweaver See my comment on #34140. When you use a custom type, it will no longer add_filter() on anything. You will handle it instead, so you can define how it accesses the value in an efficient way. I've been doing this in the Customize Widgets Plus plugin for widget settings specifically.

Also, please tone down the sarcasm and exasperation in your comments. It doesn't make us want to help you more. We're just volunteers here.

#27 @wpweaver
9 years ago

I'm not trying to be sarcastic. I will admit that it is difficult to understand all the consequences of trying to implement a custom type. I will look at your example, but I have no confidence that I will be able to make this work.

But I still do not get any impression that you understand my diagnosis if the real issue here, or that you intend to fix it for 4.4.

This is NOT all about me. It is about potentially hundreds of other themes using more than 50 or so options. The current implementation means that there are N*N operations going on when only N are needed. You can see it now in existing themes where the preview refresh takes several seconds when it need only take 1 or 2 seconds. Multiply this by tens of thousands of people using these themes and plugins, and we area talking about days and days of real people's wasted time waiting for the preview to refresh.

I feel a moral obligation as a professional computer scientist to see that this problem is fixed and done right. I don't know all the other details of how all the rest of the code works, nor what other issues need be solved.

I know everyone is a volunteer. I appreciate that. My themes, Weaver II and Weaver Xtreme, were recently rated as the #12 and #21 most used themes on all WordPress sites hosted by GoDaddy. I have a lot of users. I WANT to use the customizer. I apparently will not be able to do this until 4.4 because I can't continue development in a way that allows others to test it.

And I the only feed back I've gotten is that 600 options are too many, and what else could I expect other than problems, and that I'm being sarcastic. Bitter, perhaps, but not sarcastic.

So, this is a serious issue. I have discovered the real problem, and hope that you understand the seriousness of what it means. I don't know your programming background, but every indication I get is that who ever (and I don't assume it is you, but it could be) developed the method of forcing every option to be run through N filters on the get_option filter resulting in N*N calls ever understood what was really happening, and never gave performance a second thought.

And I really must say that the early responses to the slowness issue in this ticket really fail to provide any real solutions. They seem to be a bit of "Oh, let's try this or that" without really getting at the heart of the matter, which turns out to be the multiple instances of filters. All the rest is merely noise.

I apologize if I'm pissing you off, but this is not a personal matter, nor am I trying to attack you personally. I'm just trying to attack the real problem, and get it solved for me and hundreds of other users of the customizer. I don't want to see 4.4 come out, and still see the N*N issue and the speed problem unsolved.

#28 @westonruter
9 years ago

  • Focuses administration removed
  • Summary changed from Customizer sanitizes data multiple times when options are served as Serialized Settings to Multidimensional Customizer settings (options & theme mods) are not scalable

@wpweaver I understand the issue and how important it is to you. The code involved here has been in Core a long time (10 releases, since WordPress 3.4) and it was originally designed (not by me) without anticipating the scale that it is being used today. Be patient. You already have a workaround solution for the issue in your theme currently by implementing a custom setting type. Try that.

(BTW, I do have a computer science degree, if this gives you more confidence.)

#29 @wpweaver
9 years ago

My apologies. I have overreacted, but this is an important problem that affects the entire theme and plugin development community.

If you had acknowledged that you all understood the exponential time issue at some point, it would have helped. Instead all I heard was I use too many options, which has nothing to do with the issue.

And the algorithm used for adding the setting filters, while quite elegant code-wise, is one of the very best examples I've ever seen of a bad algorithm with significant consequences for many many people. I would guess that it has cost countless hours of time spent waiting for the refresh. It would make a nice classroom discussion for new CS students that even these days, you need to think about execution time.

With the highly appreciated helpful note from you on the other ticket, I have successfully implemented a subclass to solve the issue for me, at least fairly well. The sanitize problem may still be there, but the refresh is under 10 seconds, which is at least fairly usable.

If you want to see my implementation of the option (or theme mod) filter, I'd be happy to provide it as I think it can be generalized to solve the exponential time issue. It uses a class static variable to hold instances of the settings, and a static class function to implement the filter. I don't use the multidimension stuff to reset the values because my settings don't have arrays of arrays, but it could be used. And it only needs to be applied to preview - all the rest work okay unchanged.

Last edited 9 years ago by wpweaver (previous) (diff)

#30 @westonruter
9 years ago

  • Keywords needs-patch added; has-patch needs-testing reporter-feedback removed

I scripted a test to see how the load time grows as the number of multi-dimensional settings grows. I identified two boolean options that impact the preview time: whether or not the multidimensional setting had previously been added as an option, and whether or not the setting's value is dirty (changed, and thus needing to previewed).

From my analysis, I can see that 32103.diff only fixes the performance problems when the option is already added (e.g. add_option() was used during plugin/theme activation), and the settings are not dirty.

In the three other scenarios, the time to load the preview grows exponentially as the number of multidimensional settings are increased, just as in trunk without the patch.

Data with graphs on the performance: https://docs.google.com/spreadsheets/d/1cu5rvevr7VKgeooek9LD3J1ZD9VLTlGm-MtoppdJ78k/edit#gid=1830051232

Data obtained via scripts in this Gist: https://gist.github.com/westonruter/b4640cbf4d71bc84933f

So I do see that the original approach I was taking in 32103.wip.diff is going to be needed, where it stores the root value of multidimensional settings in a static array, and this static variable is then accessed and mutated when performing the value, preview, and update operations.

#31 @westonruter
9 years ago

  • Keywords needs-testing needs-unit-tests has-patch added; needs-patch removed

In 32103.wip2.diff, the increase in the number of multidimensional settings shows performance changes from an quadratic O(n2) to instead be a linear O(n), though actually the slope is so shallow that it is much closer to a constant O(1). In other words, instead of an exponential increase in time to render the preview when the number of multidimensional settings increase, this patch yields preview refresh times that are consistent whether there are 50 dirty settings or 500 dirty settings (200ms vs 300ms, in contrast to trunk at 650ms vs 49s, respectively).

Patch is still WIP and needs new unit tests, as well as fixing a couple broken unit tests.

#32 follow-up: @Air.
9 years ago

Nice to see you that you have got back to this issue. I don't have way to test this right now, I will try to do it soon.

I have read all new answers and I see that @wpweaver in here https://core.trac.wordpress.org/timeline?from=2015-10-03T14%3A08%3A32Z&precision=second copied exactly what I have reported in this issue, that there is problem with registered filters IDs.

As I said in this bug report I have checked that in other places in Wordpress you use more unique IDs for filters like

add_filter( 'option_nice_theme_settings[setting3]', 'function' );

, but not in this case. BY unique I mean adding part [setting3]

I still didn't see any answer why couldn't we switch to same more unique IDs here also?
Wouldn't this solve most part of issue?

Last edited 9 years ago by Air. (previous) (diff)

#33 in reply to: ↑ 32 @westonruter
9 years ago

Replying to Air.:

As I said in this bug report I have checked that in other places in Wordpress you use more unique IDs for filters like

add_filter( 'option_nice_theme_settings[setting3]', 'function' );

, but not in this case. BY unique I mean adding part [setting3]

I still didn't see any answer why couldn't we switch to same more unique IDs here also?
Wouldn't this solve most part of issue?

No, unfortunately. Assume there is a sidebar option that contains width and height keys. You then register a multidimensional setting sidebar[width] for that option, and a corresponding number control. Inside the theme, the way that you access the sidebar width is via:

$sidebar = get_option( 'sidebar' );
$width = $sidebar['width'];

You can see here that the filters only apply at the root level. There is no facility to make a call like get_option('sidebar[width]'), and so there's no filter like option_sidebar[width] to apply.

So we have to filter the settings at the root level, which currently in Core causes a problem because there are many filters added for the root value as you identified. So the approach that 32103.wip2.diff takes is it only adds one filter for the root value, and then caches the root value and any previewed modifications.

@westonruter
9 years ago

Fixed unit tests

#34 @westonruter
9 years ago

@wpweaver Please test 32103.4.diff.

#35 @valendesigns
9 years ago

I've been following the conversation for a while now. Although I haven't tested 32103.4.diff yet, I reviewed the code/logic and the patch looks really good! I'm eager to hear how it performs with @wpweaver throwing 600+ settings at it.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


9 years ago

#37 @westonruter
9 years ago

@wpweaver Since you emphasized the absolute seriousness of this issue and that it needed the highest priority possible, it would be very much appreciated if you verified the changes in 32103.4.diff. Thank you.

#38 @westonruter
9 years ago

  • Keywords needs-unit-tests removed

#39 @wpweaver
9 years ago

Seem totally functional, and BRILLIANT!!!!

The preview update is as about as fast as I could hope for. Went from 20+ seconds to maybe 2 or 3.

EDIT: Actually,the old version was timing out at 30 seconds - it was 20+ seconds when I had implemented only about 300 options. I've been adding more (moving from the "traditional" settings API based interface customizer), and now the old version won't work at all. So, BIG change! (Benchmarked on a 2012 Mac Pro MAMP development server.)

Using my own sub-class doesn't seem to make much difference in update with the updated class. Of course, it is still essential to use my own class for older versions (4.3) of the class, but I suppose I'll eventually just go back to the native class. Fortunately, I only need to change one line of code to switch from one the the other.

This will help LOTS of people, and save LOTS of time.

I just tested with some other themes with a fair amount of options (e.g. Customizr), and all seemed faster. Customizr went from 7 seconds for the refresh to essentially instant (1 or 2 seconds.)

Thank you!

Last edited 9 years ago by wpweaver (previous) (diff)

#40 @westonruter
9 years ago

  • Keywords commit added; needs-testing removed

#41 @westonruter
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 35007:

Customizer: Fix scalability performance problem for previewing multidimensional settings.

As the number of multidimensional settings (serialized options and theme mods) increase for a given ID base (e.g. a widget of a certain type), the number of calls to the multidimensional methods on WP_Customize_Setting increase exponentially, and the time for the preview to refresh grows in time exponentially as well.

To improve performance, this change reduces the number of filters needed to preview the settings off of a multidimensional root from N to 1. This improves performance from O(n^2) to O(n), but the linear increase is so low that the performance is essentially O(1) in comparison. This is achieved by introducing the concept of an "aggregated multidimensional" setting, where the root value of the multidimensional serialized setting value gets cached in a static array variable shared across all settings.

Also improves performance by only adding preview filters if there is actually a need to do so: there is no need to add a filter if there is an initial value and if there is no posted value for a given setting (if it is not dirty).

Fixes #32103.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


9 years ago

#44 @westonruter
9 years ago

In 35724:

Customize: Ensure that a setting (especially a multidimensional one) can still be previewed when the post value to preview is set after preview() is invoked.

  • Introduce customize_post_value_set_{$setting_id} and customize_post_value_set actions which are done when WP_Customize_Manager::set_post_value() is called.
  • Clear the preview_applied flag for aggregated multidimensional settings when a post value is set. This ensures the new value is used instead of a previously-cached previewed value.
  • Move $is_preview property from subclasses to WP_Customize_Setting parent class.
  • Deferred preview: Ensure that when preview() short-circuits due to not being applicable that it will be called again later when the post value is set.
  • Populate post value for updated-widget with the (unsanitized) JS-value in WP_Customize_Widgets::call_widget_update() so that value will be properly sanitized when accessed in WP_Customize_Manager::post_value().

Includes unit tests with assertions to check the reported issues and validate the fixes.

Fixes defect introduced in [35007].
See #32103.
Fixes #34738.

#45 @westonruter
8 years ago

In 40036:

Customize: Ensure root values are accessible in multidimensional custom setting types.

Fixes bad conditions in WP_Customize_Setting::get_root_value() and WP_Customize_Setting::set_root_value().

Props dlh.
Amends [35007].
See #32103.
Fixes #36952.

#46 @dd32
8 years ago

In 40088:

Customize: Ensure root values are accessible in multidimensional custom setting types.

Fixes bad conditions in WP_Customize_Setting::get_root_value() and WP_Customize_Setting::set_root_value().

Props dlh, westonruter.
Amends [35007].
Merges [40036] to the 4.7 branch.
See #32103.
Fixes #36952.

Note: See TracTickets for help on using tickets.