Improve commerce_line_item_delete_references() by only saving the order once

Okay, I originally went from "woah this breaks expectations" to "oh, now I get it!" Essentially the function's operation is unchanged for existing implementations. The controller sets the new flag parameter to FALSE. This allows it to only run the save operation on entities at a minimal amount. Otherwise we could run the save a whole bunch of times.

1. +++ b/modules/line_item/commerce_line_item.module
   @@ -657,8 +657,17 @@ function commerce_line_item_delete_multiple($line_item_ids) {
   + * @param bool $immediate_save
   ...
   +function commerce_line_item_delete_references($line_item, $immediate_save = TRUE) {
   
   @@ -676,12 +685,20 @@ function commerce_line_item_delete_references($line_item) {
   +          if ($immediate_save) {
   +            entity_save($entity_type, $entity);
   +          }
   

   $skip_save = FALSE ? and if false save, matches how core does order status updates.

2. +++ b/modules/line_item/commerce_line_item.module
   @@ -657,8 +657,17 @@ function commerce_line_item_delete_multiple($line_item_ids) {
   + *   Weather to save immediately or defer till a call outside.
   

   Whether* (:

Here's some more data. Order with 9 order discounts.

Without patch: https://blackfire.io/profiles/2b13e5f8-d28f-46fd-b6fc-951fea90a1a8/graph

With patch: https://blackfire.io/profiles/30190eb3-1a36-48cb-8d7e-3fe99c2e3bcc/graph

commerce_discount_order_refresh was 200ms faster.
commerce_cart_refresh was 1.3s to 894ms

There's obviously issues in Discount, but this makes it a lot better.

cart in question

But because the order entities aren't passed by reference each time and got with entity_load(),

But wouldn't this always be a risk? The loaded entities will still have hit the controller's internal static cache. The only issue is if someone edited the order and didn't commit their changes, it seems. I'm not sure how what's happening here is any different than what's existing, except we're providing a way to reduce the number of saves.

+++ b/modules/line_item/includes/commerce_line_item.controller.inc
@@ -139,9 +139,52 @@ class CommerceLineItemEntityController extends DrupalCommerceEntityController {
+            $entities = entity_load($entity_type, array_keys($data));
...
+              if (!isset($entities_to_save[$entity_type . ':' . $entity_id])) {

Do we even need to load? We have the entity type and the entity ID.

If the type and entity ID are not set, add. This way we can actually stop loading duplicates. I get it shouldn't be that big of a deal because of static caches, but it could streamline the process some.

+++ b/modules/line_item/includes/commerce_line_item.controller.inc
@@ -139,9 +139,52 @@ class CommerceLineItemEntityController extends DrupalCommerceEntityController {
+            $entities = entity_load($entity_type, array_keys($data));
...
+            foreach ($entities as $entity_id => $entity) {
...
+                  'entity' => $entity,
...
+                $entity = $entities_to_save[$entity_type . ':' . $entity_id];
...
+              commerce_entity_reference_delete($entity, $field_name, 'line_item_id', $line_item->line_item_id);
...
+      foreach ($entities_to_save as $entity_data) {
+        entity_save($entity_data['entity_type'], $entity_data['entity']);

Nevermind my last comment. This is done to ensure proper object references.

Refactored the patch slightly to remove unnecessary entity loads, otherwise I think it's good to go! Does this mean I've made it so I can't RTBC? hmm...

@joelpittet, if you still have your profiling environment up, mind running it through again?

Last one didn't upload right, here's the proper one.

Talked w/ @joelpittet, turns out to be a really really edge case for my change, so more likely to cause performance issues than fix it. Putting this to RTBC with #12

Gonna commit this light reroll with the else hunk removed from the $entities_to_save array. Added some comments further explaining the rationale for the patch at the top of the try block.

Committed.