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

ENH Cache $item->ID for eager loading #10840

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Jun 27, 2023

Issue silverstripe/silverstripe-elemental#1067

This small change makes a remarkably large difference to eager loading performance.

Every call to $item->ID in the loop will call ViewableData::__get(), which in turns ends up creating a new object via Injector. While this is a fast operation, if you have a huge number of calls, such as in my case where I was working on the highly unoptimized cms site search, with 1000 pages and 5000 content blocks, this saved over a million calls per search. All this PHP object creation was so bad it made enabling eager loading actually slower despite the drop in database calls.

This change makes it so that eager loading now faster in this particular operation, as would be expected

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably a lot of optimisation that can be done for this feature.... but in any case this PR has a very narrow scope, and I have seen first hand that this is a very clear improvement in itself, so I'm fine merging this on its own merits.

@GuySartorelli GuySartorelli merged commit 87d82ee into silverstripe:5 Jun 28, 2023
@GuySartorelli GuySartorelli deleted the pulls/5/eager-optimise branch June 28, 2023 03:07
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

Successfully merging this pull request may close these issues.

2 participants