uc_product_get_models() causes memory exhaustion with large # of SKUs

ouch. been bit by this bug too. no patch yet but taking a look at it.

To be clear, the above code is in uc_order.ca.inc inside the function uc_order_condition_has_products_form(). It is attempting to get all SKUs for all products, and the memory exhaustion is due to loading each and every product node and invoking hook_uc_product_models() for each.

@torgosPizza: Here's a patch to fix this problem. I've also attached a PHP script to measure the before/after query speed and memory usage.

Can you PLEASE run this command-line script against your very large database to prove the performance increase on a real site? The script has some lines commented out - you need to uncomment two lines for testing BEFORE the patch, then uncomment two different lines for testing AFTER the patch. Run it from your Drupal root directory. Let me know if you have any questions.

My tests on a DB with 1000 products shows that after this patch, this function only uses a fraction of a Mb, regardless of number of products, and the execution time is just 20% of the original.

Ran this on my laptop.
Before:

Starting memory usage = 64.76 Mb
Elapsed time = 46.219387054443 microseconds
There are 4230 SKUs in the database
Peak memory usage = 514.61 Mb

After:

Starting memory usage = 66.14 Mb
Elapsed time = 2.543154001236 microseconds
There are 4222 SKUs in the database
Peak memory usage = 71.78 Mb

2.5 seconds isn't really snappy but its a lot better than 46 seconds. It is seconds btw, microtime() returns a float of seconds with the microseconds as the decimal.

Is there any concern that this is changing an API?

@neclimdul: Thanks for the test.

With the patch, uc_product_get_models() now performs one query per nid, instead of needing a node_load() per nid. That's the huge savings in memory and the main savings in time. But if you loop over 4000+ nids you're still performing 4000+ queries plus another 4000+ queries if you have attributes enabled. So it scales linearly with the number of products, and it's bound to take some time for that number of queries.

Another improvement might be to make a second API function called uc_product_get_models_multiple(array $nids) which could be used to turn those first 4000 queries into one. That could cut the time in half. I don't know if it gains much to also do a hook_uc_product_models_multiple(array $nids), because you would have to do a WHERE nid IN ($nid[0], $nid[1], ...) and you can't fit 4000 nids in the WHERE clause. It would be a balancing act to see how big a chunk you'd have to split the nid array into in order to execute faster than the individual SELECTs without taking too much memory. And that chunk size might very well depend upon the database being used. The other choice is to make one query that contains a big JOIN, but again I feel that might make things worse. I'll experiment tonight with those options.

I'm not concerned about the minor API change, at least for 7.x-3.x. There are only a few contributed modules that use these two functions, and that can be taken care of with a coder upgrade rule. Best to do it now before the 7.x-3.x release though.

For 6.x-2.x, again there are only a few contributed modules. If this were Drupal core we couldn't do the API change in 6.x, but I think with Ubercart 6.x is still going to be around for several years so I don't want to put off changes that could have a big impact like this. I'd prefer to roll more frequent point releases so that any API changes can be introduced gradually. I searched the entire Drupal repository for uses of uc_product_get_models() and hook_uc_product_models(), and found only the following contributed projects:

uc_recurring
uc_affiliate2
uc_hosting
uc_product_panes

I think that's sufficiently small that we can live with just posting an issue in those queues to let the maintainers know there's been an API change.

Actually, since uc_product_get_models() is only used in an administration context, I don't see the need to bend over backwards to optimize it further. The big win is the memory savings, which make it actually possible to use this feature on sites with a large number of products. That was the original bug report. The greatly increased speed is a bonus, and a few seconds is really no big deal for loading an admin form. If the speed becomes an issue for someone in the future, we can pursue some of the ideas in #5.

Looks like we can retain backward compatibility in 6.x for the time being by detecting in uc_product_get_models() whether it is being passed an object or not.

Good idea. Here's a new D6 patch as well as a D7 patch with a coder upgrade rule for the API change.

Hmm, actually it's not entirely backward compatible as we are changing the hook signature as well. Should we rename the hook? But then again, does anything other than uc_attribute actually implement hook_uc_product_models?

From my search of the repository, only uc_attribute uses that hook, so I elected not to add in the additional overhead of the is_object() call.

Committed to 6.x. Reuploading 7.x patch for testbot.

Committed to 7.x.