Routing performance for Catalog in FindNextContentInSegmentPair

Vote:
 

Inspected from Episerver commerce v11.3.0.0:

namespace EPiServer.Commerce.Routing
{
	/// 
	/// Partial router for catalog content, which handles hierarchical structure
	/// 
	public class HierarchicalCatalogPartialRouter : ICommerceRouter, IPartialRouter
	{
		//...
		/// 
		/// Finds the commerce content that matches one specific segment.
		/// 
		/// The content that matches the previous segment
		/// The current segment
		/// The complete segment context.
		/// The requested cultureInfo.
		/// The content that matches the specified segment
		protected virtual CatalogContentBase FindNextContentInSegmentPair(CatalogContentBase catalogContent, SegmentPair segmentPair, SegmentContext segmentContext, CultureInfo cultureInfo)
		{
			HierarchicalCatalogPartialRouter._logger.DebugBeginMethod("FindNextContentInSegmentPair", new object[]
			{
				catalogContent,
				segmentPair,
				segmentContext,
				cultureInfo
			});
			Validator.ThrowIfNull("catalogContent", catalogContent);
			if (catalogContent.ContentType == CatalogContentType.Root || catalogContent.ContentType == CatalogContentType.Catalog)
			{
				return this._contentLoader.GetChildren(catalogContent.ContentLink, cultureInfo).FirstOrDefault((CatalogContentBase x) => segmentPair.Next.Equals(x.RouteSegment, StringComparison.OrdinalIgnoreCase));
			}
			return this._contentLoader.GetBySegment(catalogContent.ContentLink, segmentPair.Next, cultureInfo) as CatalogContentBase;
		}
		//...
	}
}

I came across this function when chasing performance problems, getting all the children to a catalog is very costly if everything isn't in the cache.

Does anyone know why it can't use _contentLoader.GetBySegment if the parent is a catalog?

GetBySegment results in a stored procedure call, mdpsp_GetChildBySegment, and in my testing it works as intended to retrieve a node or entry directly beneath the catalog.

#197364
Oct 01, 2018 16:35
Vote:
 

Erik, why are you using 11.3? You should be using 12.9 now.

I don't know why, will look into it. But GetChildren is cached as well. Not very effective, yes, if you update entries a lot.

And thanks!

#197369
Edited, Oct 01, 2018 19:12
Vote:
 

Why indeed, got a coworker upgrading to last 11 version as we speak and got 12.9 planned after that gets deployed, just have to try and keep it prioritized. wink

Yes the performance issue we have seen has mostly been on the administration server and it got much worse recently, windows update and we suspect it has -somehow- messed up memory/cache.

Still when entries/nodes gets updated and we miss the cache we rather take the small hit of GetBySegment on multiple requests than the big hit from GetChildren on a single request. embarassed

#197407
Oct 02, 2018 12:41
Vote:
 

Have some good news for you: I filed a bug for it and tried the fix. All the tests passed, which is a good sign. It's however, hard to have a test case that proves this actually brings significant improvement. But I guess I can just get away with it. 

#197408
Oct 02, 2018 12:51
Vote:
 

Great, and the fix was to remove:

|| catalogContent.ContentType == CatalogContentType.Catalog

from the if-clause?

I will add it to our override of HierarchicalCatalogPartialRouter, while i wait for our upgrade. wink

That would be hard to test indeed as it is only when the cache is empty it will affect perfomance. I think i have mentioned it before but one of the reasons we get so affected is that the loading of nodes results in many db calls to fetch their assests, it is quite possible we have more assets on our nodes than most solutions.

#197411
Oct 02, 2018 13:22
Vote:
 

The fix, at this point, is to remove this 

			if (catalogContent.ContentType == CatalogContentType.Root || catalogContent.ContentType == CatalogContentType.Catalog)
			{
				return this._contentLoader.GetChildren<CatalogContentBase>(catalogContent.ContentLink, cultureInfo).FirstOrDefault((CatalogContentBase x) => segmentPair.Next.Equals(x.RouteSegment, StringComparison.OrdinalIgnoreCase));
			}

:) 

#197412
Oct 02, 2018 13:25
Vote:
 

Wow so it wasn't even needed for Root? nice! :D

#197413
Oct 02, 2018 13:26
Vote:
 

IIRC the GetBySegment implementation for the catalog content provider lacked this capability from the beginning, and since we expected the number of direct children of the catalog to be few and listed often (=cached) it didn't hurt. Then I guess at some point some other requirement drove extension of GetBySegment to include the catalog, but it wasn't used in the router.

Edit: https://world.episerver.com/support/Bug-list/bug/COM-320

It was fixes a while ago :)

#197417
Edited, Oct 02, 2018 14:57
Vote:
 

FYI - This how it looks in new relic when we miss the cache today:


Total duration     Call count     Query
4,370 ms         1326         ecf_CatalogNode_Asset
320 ms             120         mdpsp_sys_LoadMultiValueDictionary
174 ms             30             netFindContentCoreDataByContentGuid
11 ms             5             mdpsp_sys_LoadMetaFile
20 ms             2             ecf_CatalogNode_List
6 ms             2             ecfVersion_List
51 ms             2             CatalogContentProperty_LoadBatch
10 ms             1             ecf_CatalogNode_CatalogParentNode
321 ms             1             ecf_Catalog_GetChildrenEntries
106 ms             1             ecf_CatalogEntry_List
6 ms             1             ecf_NodeEntryRelations

#197534
Edited, Oct 05, 2018 13:54
Vote:
 

So it was you!

Kidding aside, the bug was fixed and will be released in the next - most likely 12.10

#197536
Oct 05, 2018 14:04
Vote:
 

Fixed in 12.10 just like Quan said laughing https://world.episerver.com/documentation/Release-Notes/ReleaseNote/?releaseNoteId=COM-8038

#198113
Oct 22, 2018 11:13
Vote:
 

I am known to keep my words ;)

#198114
Oct 22, 2018 11:25
This topic was created over six months ago and has been resolved. If you have a similar question, please create a new topic and refer to this one.