Skip to content
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

Open
MatteoPiovanelli-Laser opened this issue May 18, 2023 · 7 comments
Open

Perf: GetMany doesn't use ContentManagerSession #8688

MatteoPiovanelli-Laser opened this issue May 18, 2023 · 7 comments
Milestone

Comments

@MatteoPiovanelli-Laser
Copy link
Contributor

public IEnumerable<T> GetMany<T>(IEnumerable<int> ids, VersionOptions options, QueryHints hints) where T : class, IContent {

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 (

localField._contentItems = new Lazy<IEnumerable<MediaPart>>(() => _contentManager.GetMany<MediaPart>(localField.Ids, VersionOptions.Published, QueryHints.Empty).ToList());
) 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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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

@sebastienros sebastienros added this to the 1.10.x milestone May 25, 2023
@BenedekFarkas
Copy link
Member

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

@MatteoPiovanelli-Laser
Copy link
Contributor Author

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

@MatteoPiovanelli-Laser
Copy link
Contributor Author

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants