-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Perf: GetMany doesn't use ContentManagerSession #8688
Comments
Correct. I am pretty sure we do that in OC. |
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) |
Further testing: |
Has anyone looked more into this yet maybe with a PoC change? I would love to have this fixed! |
I haven't had a chance yet. Hopefully over the next few weeks. |
I did some test during a couple meetings. |
Sounds great! I'll test it with an application that uses Taxonomies extensively once you've got the PR submitted. |
Orchard/src/Orchard/ContentManagement/DefaultContentManager.cs
Line 284 in 610b3c4
I was checking the performances of some test pages. In one of them I have a bunch of
MediaLibraryPickerField
s. 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 (
Orchard/src/Orchard.Web/Modules/Orchard.MediaLibrary/Handlers/MediaLibraryPickerFieldHandler.cs
Line 40 in 610b3c4
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
ContentPickerField
s 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.
The text was updated successfully, but these errors were encountered: