If you are using Optimizely Customized Commerce, the common wisdom is that you should be using serializable cart mode. It’s not perfect (we discussed the drawbacks in, um, my book), but generally it has performance benefits. But for any reason that you need to use the legacy cart mode, there is a switch you can use – IFeatureSwitch which can be used to change between those modes
It is important to remember that IFeatureSwitch only switch between to modes, it does not migrate any carts between those two. there is a one time cart migration job but that’s it.
To let IOrderRepository use the correct cart system, there is an ICartProvider which will be either CartProvider or SerializableCartProvider . The problem is that happens much earlier in the pipeline than IInitializationModule. In fact it is determined in IConfigurableModule.ConfigureContainer , which means before any IInitializationModule.Initialize. Even if we call DisableFeatures in another ConfigureContainer, there is no warranty that our code will be called before the framework code (the order of ConfigureContainer execution is indeterministic )
But fortunately, we can do that inside Startup.Configure. Due to how the feature switch data structure, it’s not as simple as adding a setting in appsettings.json, but it can be done easily in code:
services.Configure<ApplicationOptions>(x =>
{
x.Features[SerializedCarts.FeatureSerializedCarts] = new ApplicationFeature
{
Feature = "SerializedCarts",
State = FeatureState.Disabled,
Type = "Mediachase.Commerce.Core.Features.SerializedCarts, Mediachase.Commerce"
};
});
Of course, this is a workaround. The feature switch should be done as documented. It will be fixed in the future.
While we always want to have fast database queries, it is not doable all the time. Sometimes we need to run slow queries, and we need to tell the underlying framework that this query can take some time to complete, and we’re fine with it. Otherwise, it will try to terminate the query after 30 seconds (the default time out limit)
There is a different between connection timeout and command timeout. Connection timeout is the time .NET will try to connect to the database before giving up. Command timeout is the time .NET will try to execute a command before giving up.
Things are a bit complicated when it comes to command timeout with .NET 5 and up. With later versions of Microsoft.Data.SqlClient, it is possible to set command timeout directly using connection string. It is indeed a simple way to do it, but with a caveat.
The new setting is not recognized by Entity Framework/Entity Framework Core, and it will throw exception if you try to access a connection string with command timeout setting. It has another way to set the command timeout itself by each DbContext , but it does not accept the setting via Connection string. It will throw “Keyword not supported: ‘command timeout'” if such setting is present.
The workaround is to configure the command timeout for EcfSqlConnection connection string, and another different connection string without command timeout just for Entity Framework.
However, that’s with a caveat: using command timeout in connection string means that value applies to all database queries. As we discussed in the previous post above, doing so is not without drawbacks – it hides slow queries rather than let it fails. A failed query might not bring down your website, but an overloaded database will likely do.
In Commerce 14.15.6 which should be released shortly, we introduce a new setting SqlDataProviderOptions.CommandTimeout which let you set the command timeout for queries that are using SqlDataProvider – most notably the MetaDataPlus system like orders.
The important advantage of the new setting is that you can set it on the fly. If you know that some operation will be slow, you can set the command timeout to a higher value just for that operation, then set it back to default value. In most cases, you can leave it to default value (30 seconds), and do optimization on application level (reduce batch size for example) or database layer (rewrite the query, adding/optimizing indexes etc.). But sometimes you know the query would be slow and you acknowledge that – this is the tool.
Let’s take a break from the memory allocation, and do some optimization on another aspect, yet as important (if not even more important) – database.
We all know that database queries play an essential part in any serious app. It’s almost a given that if you want your app to perform well, your database queries must also perform well. And for them to perform well, you need things like proper design (normalization, references etc.), properly written queries, and proper indexes. In this post, we will explore how an index can improve query performance, and how can we do it better.
Let’s start with a fairly simple table design
CREATE TABLE [dbo].[UniqueCoupon](
[Id] [int] identity primary key clustered,
[PromotionId] [int] NOT NULL,
[Code] [nvarchar](10) NOT NULL,
[ExpiredOn] [datetime] NULL,
[Redeemed] [bit] NULL
) ON [PRIMARY]
Nothing extraordinary here, pretty common if you ask me. Now for testing purpose, let’s insert 1.000.000 rows into it
INSERT INTO dbo.[UniqueCoupon] (PromotionId, Code)
SELECT
FLOOR(RAND()*(100)+1),
SUBSTRING(CONVERT(varchar(255), NEWID()), 0, 7)
GO 1000000
We need to query data by the code, so let’s create an user defined type
CREATE TYPE CouponTable AS TABLE (
Code NVARCHAR(10));
Time to run some query against data, let’s go with this
SELECT Id, PromotionId, Code, ExpiredOn, Redeemed FROM dbo.UniqueCoupons
WHERE PromotionId = @PromotionId AND Code in (SELECT Code FROM @Data)
This is the complete query as we need some data
declare @data CouponTable
insert into @data
select top 10 code from dbo.UniqueCoupon
where promotionid = 36
SELECT Id, PromotionId, Code, ExpiredOn, Redeemed FROM dbo.UniqueCoupon
WHERE PromotionId = 36 AND Code in (SELECT Code FROM @Data)
As we learned that execution plan is not a good way to compare performance, let’s use the statistics, our trusted friends
set statistics io on
set statistics time on
And this is how it takes with our default setting (i.e. no index)
If you are somewhat experienced with SQL Server, you might guess it would not be exactly happy because of, obviously an index is needed. As we query on PromotionId, it does makes sense to add an index for it, SQL Server does give you that
If we just blindly add the index suggested by SQL Server
If we look at the index, there’s something not very optimized about it – we are query by both PromotionId and Code, so not really makes senses to have Code as included. How’s about we have the index on both PromotionId and Code?
Yet we can make it better! From 53 to 30 logical reads might not sound a lot, but if you have thousands of queries every hour, it will be fairly significant.
Prepare yourself for some pleasant surprises – when we eventually applied the change on an actual database, the change was staggering, much more than what we hoped for. The query that were run for 24h in total, every day, now takes less than 10 minutes (yes you read it right, 10 minutes).
At this point you can certainly be happy and move on. But can we do better? For the sake of curiosity ? Yes we do.
SQL Server is rather smart that it knows we are getting the other columns as well, so those will be included in the index, to avoid a key lookup. Let’s see if we can remove that and see how it performs
So it was indeed worse, a key lookup is performed for every row (SQL Server uses the index to track down the rows and read the other columns from there)
There are two way to get rid of those key lookup – includes the columns in the index itself, or, more dramatic, make the index the clustered. As we can see the data should be accessed by PromotionId and Code, it makes perfect senses.
It is a commonly belief that Identity column should be clustered index – it is unique, it is not null. However, it only makes senses if it is the most heavily accessed column. In this case, Id only serves as an Identity column, it does not need to be the clustered index (although being an unique means it will has a non clustered index for it)
ALTER TABLE [dbo].[UniqueCoupon] DROP CONSTRAINT [PK__UniqueCo__3214EC0744C2FF38] WITH ( ONLINE = OFF )
GO
ALTER TABLE [dbo].[UniqueCoupon] ADD PRIMARY KEY NONCLUSTERED
(
[Id] ASC
)
Does this bring dramatically performance change? Unlikely. My test show no improvement in statistic. However, there is one critical impact here: we significantly reduced the size of indexes in the table. (data incoming)
Moral of the story
Indexes are crucial.
You can almost always do better than the auto suggested indexes.
“In 99% of the cases, premature optimization is the root of all devil”
This quote is usually said to be from Donald Knuth, usually regarded as “father of the analysis of algorithms”. His actual quote is a bit difference
We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil.
Yet we should not pass up our opportunities in that critical 3%.
If you have read my posts, you know that I always ask for measuring your application before diving in optimization. But that’s not all of the story. Without profiling, your optimization effort might be futile. But there are things you can “optimize” right away without any profiling – because – they are easy to do, they make your code simpler, easier to follow, and you can be certain they are faster.
Let’s see if you can spot the potential problematic piece of code from this snippet
public Something GetData()
{
var market = list.FirstOrDefault(x => x.MarketId == GetCurrentMarket().MarketId)
{
//do some stuffs
}
}
If you are writing similar code, don’t be discouraged. It’s easy to overlook the problem – when you call FirstOrDefault, you actually iterate over the list until you find the first matching element. And for each and every of that, GetCurrentMarket() will be called.
Because we can’t be sure when we will find the matching element, it might be the first element, or the last, or it does not exist, or anywhere in between. The median is that GetCurrentMarket will be half called half the size of list
We don’t know if GetCurrentMarket is a very lightweight implementation, or list is a very small set, but we know that if this is in one very hot path, the cost can be (very) significant. These are the allocations made by said GetCurrentMarket
This is a custom implementation of IMarketService – the default implementation is much more lightweight and should not be of concern. Of course, fewer calls are always better – no matter how quick something is.
In this specific example, a simple call to get the current market and store it in a local variable to be used in the scope of the entire method should be enough. You don’t need profiling to make such “optimization” (and as we proved, profiling only confirm our suspect )
Moral of the story
For optimization, less is almost always, more
You definitely should profile before spending any considerable amount optimizing your code. But there are things that can be optimized automatically. Make them your habit.
Earlier we started a new series about performance optimization, here Performance optimization – the hardcore series – part 1 – Quan Mai’s blog (vimvq1987.com) . There are ton of places where things can go wrong. A seasoned developer can, from experience, avoid some obvious performance errors. But as we will soon learn, a small thing can make a huge impact if it is called repeatedly, and a big thing might be OK to use as long as it is called once.
Let’s take this example – how would you think about this snippet – CategoryIds is a list of string converted from ContentReference
if (CategoryIds.Any(x => new ContentReference(x).ToReferenceWithoutVersion() == contentLink))
{
//do stuff
}
If this is in any “cool” path that run a few hundred times a day, you will be fine. It’s not “elegant”, but it works, and maybe you can get away with it. However, if it is in a hot path that is executed every time a visitor visits a product page in your website, it can create a huge problem.
And can you guess what it is?
new ContentReference(string) is fairly lightweight, but if it is called a lot, this is what happen. This is allocations from the constructor alone, and only within 220 seconds of the trace
A lot of allocations which should have been avoided if CategoryIds was just an IEnumerable<ContentReference> instead of IEnumerable<string>
For comparison, this is how 10.000 and 1000.000 new ContentReference would allocate
Thing is similar if you use .ToReferenceWithoutVersion() to compare to another ContentReference (although to a lesser extend as ToReferenceWithoutVersion would return the same ContentReference if the WorkId is 0, and it use cloning instead of new). The correct way to compare two instances of ContentReference without caring about versions, is to use .Compare with ContentReferenceComparer.IgnoreVersion
Moral of the story
It is not only what you do, but also how you do it
Small things can make big impacts, don’t guess, measure!
With Find-backed IEntrySearchService in the previous post , we can now put SearchProvider to rest. There are, however, parts of the framework that still rely on SearchManager, and it expects a configured, working SearchProvider. The Full search index job, and the Incremental search index job are two examples. To make sure we don’t break the system, we might want to give SearchManager something to chew on. A do nothing SearchProvider that is!
And we need a DoNothingSearchProvider
public class DoNothingSearchProvider : SearchProvider
{
public override string QueryBuilderType => GetType().ToString();
public override void Close(string applicationName, string scope) { }
public override void Commit(string applicationName) { }
public override void Index(string applicationName, string scope, ISearchDocument document) { }
public override int Remove(string applicationName, string scope, string key, string value)
{ return 42; }
public override void RemoveAll(string applicationName, string scope)
{
}
public override ISearchResults Search(string applicationName, ISearchCriteria criteria)
{
return new SearchResults(new SearchDocuments(), new CatalogEntrySearchCriteria());
}
}
And a DoNothingIndexBuilder
public class DoNothingIndexBuiler : ISearchIndexBuilder
{
public SearchManager Manager { get; set; }
public IndexBuilder Indexer { get; set; }
public event SearchIndexHandler SearchIndexMessage;
public void BuildIndex(bool rebuild) { }
public bool UpdateIndex(IEnumerable<int> itemIds) { return true; }
}
What remains is simply register them in your appsettings.json
If you have been using Find, you might be surprised to find that CSR UI uses the SearchProvider internally. This is a bit unfortunate because you likely are using Find, and that creates unnecessary complexity. For starter, you need to configure a SearchProvider, then you need to index the entries, separately from the Find index. If you install EPiServer.CloudPlatform.Commerce, it will setup the DXPLucenceSearchProvider for you, which is basically a wrapper of LuceneSearchProvider to let it work on DXP (i.e. Azure storage). But even with that, you have to index your entries anyway. You can use FindSearchProvider, but that actually just creates another problem – it uses a different index compared to Find, so you double your index count, yet you have still make sure to index your content. Is there a better way – to use the existing Find indexed content?
Yes, there is
Searches for entries in CSR is done by IEntrySearchService which the default implementation uses the configured SearchProvider internally . Fortunately for us, as with most thing in Commerce, we can create our own implementation and inject it. Now that’s with a caveat – IEntrySearchService is marked as BETA remark, so prepare for some breaking changes without prior notice. However it has not changed much since its inception (funny thing, when I checked for its history, I was the one who created it 6 years ago, in 2017. Feeling old now), and if it is changed, it would be quite easy to adapt for such changes.
IEntrySearchService is a simple with just one method:
It is a bit weird to return an IEnumerable<int> (what was I thinking ? ), but it was likely created as a scaffolding of SearchManager.Search which returns an IEnumerable<int>, and was not updated later. Anyway, an implementation using Find should look like this:
Note that I am not an expert on Find, especially on NestedFilterExpression, so my FilterPriceAvailableForCurrency might be wrong. Feel free to correct it, the code is not copyrighted and is provided as-is.
As always, you need to register this implementation for IEntrySearchService. You can add it anywhere you like as long as it’s after .AddCommerce.
Carts are meant to be validated. Prices changed, customers add more quantity than allowed, promotions expired, stock ran out, etc.. All kinds of stuffs that make the items in carts need validation to make sure they up to date, and be ready to be converted to an order.
However, there are cases when you don’t want your carts to be validated. The most common case is of course, wish list – a special cart that allows customer to add items to, just to keep track of. You certainly don’t want to touch it. Another example is quote – when you give a specific item at specific price for a customer, and you don’t want it to be automatically changed to the public prices, which is different from that said price.
By default, when it is called to validate a cart, these things will be done:
Remove items that no longer available (either deleted or end of line)
Update prices of the items to the latest applicable prices, or remove items that have no prices.
Update quantity of the items (to comply with the settings or in stock quantity), or remove items that are out of stock
Apply promotions
Update quantity again (As promotions could do things like adding free items to the cart)
There are two ways you can avoid carts being validated, let’s see what we can do.
The “Wish list names” route
With OrderOptions you can set certain wishlist names to be exempted from the validation, using WishListCartNames. By default, it’s only “Wishlist”, but you can set several using the comma separator, like this
However there is a caveat, with this approach, carts in those names will not be shown in the Order Management (If you want, you can change that, however it is not an easy or quick one)
The OrderValidationService route
The validation of carts (or rather, order types in general) is done by OrderValidationService. And that class is meant to be extended if necessary. Here is how you would avoid validation carts with name “Quote”, using OrderValidationService
public class CustomOrderValidationService : OrderValidationService
{
public CustomOrderValidationService(ILineItemValidator lineItemValidator, IPlacedPriceProcessor placedPriceProcessor, IPromotionEngine promotionEngine, IInventoryProcessor inventoryProcessor, OrderOptions orderOptions) : base(lineItemValidator, placedPriceProcessor, promotionEngine, inventoryProcessor, orderOptions)
{
}
public override IDictionary<ILineItem, IList<ValidationIssue>> ValidateOrder(IOrderGroup orderGroup)
{
if (orderGroup.Name.Equals("Quote", System.StringComparison.OrdinalIgnoreCase))
{
return new Dictionary<ILineItem, IList<ValidationIssue>>();
}
return base.ValidateOrder(orderGroup);
}
}
And as OrderValidationService is registered by ServiceConfiguration attribute, you can register yours by
One caveat though, making changes to OrderValidationService means those changes will apply to the entire website, so make sure the changes are actually the ones you want site-wide, not just in specific places.
If you are using Find to index your content, you likely have used the Find Indexing job – which would index everything in one go. Today I stumped upon this question – A way to run indexing job for Commerce only | Optimizely Develope – and it is a good one – if you have many of content in CMS side, and they don’t change that often, if at all – you certain don’t want to waste time and resource in trying to reindex them again. Is there away to just index catalog content?
Yes, there is. It is a bit hacky solution, but it can certain work. But first, let’s dive in on how Find indexing job does it work. It relies on IIndexingJobService , which itself relies on ContentIndexer to do the job. In its turn, ContentIndexer uses a list of IReindexInformation to know which content to index, and in which languages. Here’s what it looks like
public interface IReindexInformation
{
/// <summary>
/// Content links to be reindexed.
/// </summary>
IEnumerable<ReindexTarget> ReindexTargets { get; }
/// <summary>
/// Gets the root to index.
/// </summary>
ContentReference Root { get; }
}
It has one Root, and multiple ReindexTarget, which contains
public class ReindexTarget
{
/// <summary>
/// The content references.
/// </summary>
public IEnumerable<ContentReference> ContentLinks { get; set; }
/// <summary>
/// The languages the collection of <see cref="ContentReference"/> are enabled on.
/// </summary>
public IEnumerable<CultureInfo> Languages { get; set; }
/// <summary>
/// The site that the collection of <see cref="ContentReference"/> appears on
/// or <c>null</c> if unknown.
/// </summary>
public SiteDefinition SiteDefinition { get; set; }
}
As you might have guessed, Commerce has its own IReindexInformation to index catalog content. If we can only use that to run our job. This is how our “hack” begins
The interface IContentIndexer has no method to control the IReindexInformation`, but the default implementation ContentIndexer does. We set it to the only one we need, so here it is
List<IReindexInformation> targets;
var contentIndexer = _contentIndexer as ContentIndexer;
if (contentIndexer != null)
{
targets = contentIndexer.ReindexInformation.ToList();
var commerceReIndexInformation = targets.FirstOrDefault(x => x.GetType() == typeof(CommerceReIndexInformation));
contentIndexer.ReindexInformation = new List<IReindexInformation>() { commerceReIndexInformation };
_indexingJobService.Start(OnStatusChanged);
contentIndexer.ReindexInformation = targets;
}
A note is that you will still see the “Indexing Global assets and other data” message, because IIndexingJobService implementation will go through all SiteDefinition regardless and show that message, but the internal ContentIndexer will skip if the SiteDefinition passed to it does not match the SiteDefinition in the IReindexInformation (and for CommerceReIndexInformation it’s SiteDefinition.Empty
As I mentioned in the beginning, this is a bit hacky solution, as you have to cast IContentIndexer to its concrete implementation. The proper solution would be implement IContentIndexer yourself. Given that’s not a trivial job, I’ll leave at that.
A while back, I wrote this post A curious case of cookie threading issue – Quan Mai’s blog (vimvq1987.com) as an example of how to use WinDbg to diagnose a race condition (with results in an infinite loop). Naturally that issue needs to be fixed. A starting point to fix a race condition is to put locks whenever you change the data. But it’s not that simple. Too much locking would definitely hurt your application performance, and increase the chance of you running into another nasty problem (namely, deadlock). So the art of locking is “lagom” – Swedish word for “just right”, not too much, not too little.
ReaderWriteLockSlim comes to rescue. You only need to lock when writing, not locking. So this for reading
0:039> !clrstack
OS Thread Id: 0x1e34 (39)
Child SP IP Call Site
000000740273cff0 00007ff9297d9f59 System.Collections.Generic.HashSet`1[[System.__Canon, mscorlib]].Remove(System.__Canon)
000000740273d080 00007ff92a8eb4e3 System.Web.HttpCookieCollection.Get(System.String)
000000740273d0c0 00007ff9314de98d
and look closer. Something is not right. If you do not spot the issue – it’s fine, I missed it too. The problem is that we have a HashSet.Remove inside a HttpCookieCollection.Get. Uh oh.
If we look at source code of HttpCookieCollection, inside Get, it calls to EnsureKeyValidated
if (cookie != null) {
EnsureKeyValidated(name, cookie.Value);
}
_keysAwaitingValidation is a HashSet<string> . That explains the Remove we saw. And that explains why ReaderWriterLockSlim is not enough – changes are made within the supposedly read only action as well.
The only valid solution here is to lock both read and write actions on HttpCookieCollection. However, as HttpCookieCollection is per request, so our lock object should only be per request as well (we certainly do not want every thread to be locked when we get cookie on a request).
Moral of the story:
Look closer. There might always be something underneath. Close to the truth is still not the truth.
Never assume that a Get method is thread-safe. The implementation can do plenty of unexpected things under the hood.
You might ask why this happen, as HttpCookieCollection is per request. Turned out that there is a code that use ThreadPool.QueueUserWorkItem to queue tasks which share the same HttpContext object. As we learned, that’s the recipe for disaster. Think twice (or thrice) before sharing an object that is not thread-safe between threads.
