search

OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.37k stars 1.12k forks source link

Perf: GetMany doesn't use ContentManagerSession #8688

Open MatteoPiovanelli-Laser opened 1 year ago

MatteoPiovanelli-Laser commented 1 year ago

https://github.com/OrchardCMS/Orchard/blob/610b3c4f53ce9a4bdbe26a5a095bd606bfabc811/src/Orchard/ContentManagement/DefaultContentManager.cs#L284

I was checking the performances of some test pages. In one of them I have a bunch of MediaLibraryPickerFields. Since this is only a test tenant on a dev box, all those fields have the same Media items selected.

I noticed that the query corresponding to the LazyField fetching the information for the Media from the db (https://github.com/OrchardCMS/Orchard/blob/610b3c4f53ce9a4bdbe26a5a095bd606bfabc811/src/Orchard.Web/Modules/Orchard.MediaLibrary/Handlers/MediaLibraryPickerFieldHandler.cs#L40) is fired for every field, always with the same parameters.

This is unlike fetching ContentItems one by one using the Get method, where the ContentManagerSession prevents refiring the same query multiple times.

Looking at the code, I think ContentPickerFields have the same issue.

I think GetMany should only be doing the query if any of the Ids provided to it corresponds to a ContentItem that IS NOT available in ContentManagerSession. It should still, of course, not change the order of returned items.

sebastienros commented 1 year ago

I think GetMany should only be doing the query if any of the Ids provided to it corresponds to a ContentItem that IS NOT available in ContentManagerSession

Correct. I am pretty sure we do that in OC.

sebastienros commented 1 year ago

And that's important for eager loading techniques (pre-loading a bunch of items knowing that there will be a SELECT N+1 to come)

MatteoPiovanelli-Laser commented 1 year ago

Further testing: TaxonomyField seems to be affected by the same issue.

BenedekFarkas commented 1 year ago

Has anyone looked more into this yet maybe with a PoC change? I would love to have this fixed!

MatteoPiovanelli-Laser commented 1 year ago

I haven't had a chance yet. Hopefully over the next few weeks.

MatteoPiovanelli-Laser commented 1 year ago

I did some test during a couple meetings. I think I have a working version, but I'm not sure about its performances. I'll draft a PR early next week.

BenedekFarkas commented 1 year ago

Sounds great! I'll test it with an application that uses Taxonomies extensively once you've got the PR submitted.