Unnecessary database queries when working with LineItem Adjustments

AugustMiller commented 3 years ago

Description

I was doing a bit of optimization work on Order-related views and logic for an application we maintain that historically has pretty large carts… oftentimes dozens of items—but occasionally hundreds—with many Adjustments on each.

The site also depends heavily on Line Item-level Adjustments, so being able to display them in-situ is important.

When profiling our Cart page after eliminating some N+1 element queries (i.e. Line Items -> Variants -> Products), I noticed one last cascade of queries, appearing to stem from LineItems::getLineItemById().

Despite earlier in the request the Order loading all its Adjustments, calling LineItem::getAdjustments() would in turn call (for each of its Adjustments) Adjustment::getLineItem(). Subsequent loops over the cart or its LineItems and Adjustments wouldn't, because the LineItem is memoized on the Adjustment:

https://github.com/craftcms/commerce/blob/develop/src/models/OrderAdjustment.php#L191-L201

When Adjustments are applied in-memory (say, by recalculation—not just loaded from the database), Commerce takes care of calling OrderAdjustment::setLineItem($item) to bind them together, which avoids the query—even when doing so results in the ID of a persisted Line Item being set.

A potential patch for this would be to iterate over the Adjustments when they're loaded en masse from the Order, and call $adjustment->setLineItem($lineItem), proactively. Here's how I've implemented it in a Behavior:

<?php
namespace modules\mymodule\behaviors;

use yii\base\Behavior;
use craft\helpers\ArrayHelper;
use craft\commerce\elements\Order as OrderElement;

/**
 * Extra Order-related features and convenience methods.
 * 
 * @property OrderElement $owner
 */
class OrderBehavior extends Behavior
{
    /**
     * Load Adjustments and ensure they're associated with LineItems.
     * 
     * Only operates on LineItems with IDs (previously saved), and Adjusters with a LineItem ID.
     */
    public function bindLineItemAdjustments()
    {
        // The `index` method groups the Adjustments by their `lineItemId` attribute, discarding anything with an unset/null value:
        $adjustmentsByLineItemId = ArrayHelper::index($this->owner->getAdjustments(), null, 'lineItemId');

        // Same with LineItems... sort of opaque, but the pattern holds—anything with an ID gets dumped. Could just as easily be done as part of the foreach loop.
        $lineItems = ArrayHelper::index($this->owner->getLineItems(), 'id');

        foreach ($lineItems as $lineItem) {
            // There may not be Adjustments for every LineItem, so return an empty array if the key isn't present:
            $lineItemAdjustments = $adjustmentsByLineItemId[$lineItem->id] ?? [];

            foreach ($lineItemAdjustments as $lineItemAdjustment) {
                $lineItemAdjustment->setLineItem($lineItem);
            }
        }
    }
}

At the moment, we're just calling this prior to doing any major logical or view work with LineItems or their Adjustments. In readily-available test cases, this knocked 100+ queries off our Cart and Order pages (>50% reduction)!

Happy to issue a PR, if it'd be clearer to see the code in-context!

Steps to reproduce

Add an item (or many items) to a cart
Verify an Adjustment is applied at the LineItem level
Profile the request and see a repeated query like the following:

SELECT "id", "options", "price" -- ...and all the other columns!
FROM "commerce_lineitems" "lineItems"
WHERE "id"=12345 -- Or, typically, an incrementing value, one query after the other.
LIMIT 1

Additional info

Craft CMS version: 3.7.13
Craft Commerce version: 3.4.2
PHP version: 7.4.22
Database driver & version: Postgres 13
Plugins & versions: Cookies, Mailgun, Stripe, Redactor, Typogrify

lukeholder commented 3 years ago

@AugustMiller Thanks for reporting this.

Do you think it would just make sense to add eager loading of the lineitem's adjustments to this either/both of these:

https://github.com/craftcms/commerce/blob/develop/src/services/OrderAdjustments.php#L237-L260 and https://github.com/craftcms/commerce/blob/develop/src/services/LineItems.php#L407-L430

I am guessing is is most slow on the order edit page on initial load, not for recalculations since they should all be related in memory when the order recalculates?

AugustMiller commented 3 years ago

Wow, I had no idea these methods existed (or that it could be automated via OrderQuery params)!

While these can eliminate a top-level N+1, I still think the issue still presents, because upon setting one or the other, the system makes no attempt to match things up by existing IDs.

Right, recalculation doesn't matter because Adjustments are discarded every time… this is just for outputting Order data in receipts or as a list in a Customer's account.

For context: we have an Orders dashboard that outputs a text “summary” of each Order, containing its Purchasables and one or more "Licenses" associated with each (via options), but information about those Licenses is most clearly/accurately communicated by displaying the Adjustments that were applied as a a result of the configuration. So, while eager-loading of Adjustments and LineItems will be an awesome improvement, we'll still need our custom logic (or a change in core) to pair them up upon assignment.

I'm not entirely sure whether this has to happen bidirectionally (probably yes?), and/or whether that means requesting withAdjustments or withLineItems also quietly loads both sets of records…

Succinctly, I think the issue is more with Order::setLineItems() or Order::setAdjustments()—eager-loading isn't enough to make sure Adjustments don't subsequently go back to the database for their LineItems.

craftcms / commerce