Routing performance for Catalog in FindNextContentInSegmentPair

Erik Norberg

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

Quan Mai

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

Erik Norberg

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.

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.

#197407

Oct 02, 2018 12:41

Quan Mai

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

Erik Norberg

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.

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

Quan Mai

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

Erik Norberg

Vote:

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

#197413

Oct 02, 2018 13:26

Magnus Rahl

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

Erik Norberg

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

Quan Mai

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

Johan Book

Vote:

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

#198113

Oct 22, 2018 11:13

Quan Mai

Vote:

I am known to keep my words ;)

#198114

Oct 22, 2018 11:25

Try our conversational search powered by Generative AI!

Routing performance for Catalog in FindNextContentInSegmentPair