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

IBX-7911: Used strict getters for content tree loading code paths #347

Merged
merged 20 commits into from
Jun 5, 2024

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Mar 14, 2024

Question Answer
JIRA issue IBX-7911
Type bug
Target Ibexa version v4.6
BC breaks no

We've known for quite some time that magic property getter __get on our ValueObject doesn't scale very well as it's time consuming and creates significant overhead.

This PR adds strict getters for Content, ContentInfo, Location, VersionInfo, ContentType\FieldDefinition, User\User, Security\User, SimplifiedRequest and documents deprecation of corresponding @property-read entries. It also refactors usages of magic getters found in code paths executed when loading the content tree via AdminUi\ContentTreeController::loadSubtreeAction.

The Blackfire profile shows that for ~170 calls to Ibexa\AdminUi\Siteaccess\SiteaccessResolver getSiteAccessesListForLocation, there's about 184 000 calls to magic getter, taking more than 1s of total execution time for a very big content tree:

image

The general recommendation would be to introduce a Rector which would add getters in place of @property-read and refactor usage, however this PR solves part related to the performance issue at hand.

Review notes

Added ContentDomainMapper::mapPersistenceContentTypeToApi method to translate legacy usage of persistence content type into API content type. Forcing getters in buildDomainFields method makes passing directly persistence Content Type not feasible (properties match, however persistence one has public properties, therefore there are no getters)

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly.
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review.

Copy link

sonarcloud bot commented Mar 15, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@alongosz alongosz force-pushed the ibx-7911-drop-magic-getters branch 2 times, most recently from fb4bf3f to e23f8d0 Compare May 23, 2024 13:40
@alongosz alongosz marked this pull request as ready for review May 23, 2024 14:02
@alongosz alongosz requested review from mikadamczyk, Steveb-p, tischsoic and a team May 23, 2024 14:17
@alongosz alongosz force-pushed the ibx-7911-drop-magic-getters branch 2 times, most recently from a5f4540 to cc1dc55 Compare May 24, 2024 13:15
@alongosz alongosz force-pushed the ibx-7911-drop-magic-getters branch from cc1dc55 to 6f2708b Compare May 24, 2024 13:16
@alongosz alongosz requested review from Steveb-p, konradoboza and a team May 24, 2024 13:52
Copy link

sonarcloud bot commented May 24, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.4% Duplication on New Code

See analysis details on SonarCloud

@alongosz
Copy link
Member Author

alongosz commented May 24, 2024

Update via 7869457: seems Location::$path magic property is special and is computed on-the-fly if missing.
I had to:

  1. move that logic to the strict getter Location::getPath(),
  2. move the property itself from child classes (implemenetations of Location and TrashItem) to the base class,
  3. move magic regarding that property to the base class as well and actually fallback to the strict getter....

Discovered by AdminUI REST integration tests for content tree endpoint 🎉 (ibexa/admin-ui#1212)

1) Ibexa\Tests\Integration\AdminUi\REST\PostPostLoadSubtreeTest::testEndpoint with data set "POST /api/ibexa/v2/location/tree/load-subtree accepting json format with ContentTreeLoadSubtreeRequest json payload" (Ibexa\Contracts\Test\Rest\Request\Value\EndpointRequestDefinition Object (...))
TypeError: Return value of Ibexa\Contracts\Core\Repository\Values\Content\Location::getPath() must be of the type array, null returned

*
* @var string[]
*/
protected array $path;
Copy link
Member

Choose a reason for hiding this comment

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

Idea for improvement in 5.x: introduce dedicated Value Object representing path

@micszo micszo self-assigned this Jun 3, 2024
Copy link
Contributor

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Compared time needed to load content tree with and without changes from https://issues.ibexa.co/browse/IBX-7911 & https://issues.ibexa.co/browse/IBX-8280.

On different attempts observed an improvement between 20% and 30%.

QA Approved on Ibexa Commerce 4.6.7-dev.

@micszo micszo removed their assignment Jun 5, 2024
@alongosz alongosz merged commit 5848b1e into 4.6 Jun 5, 2024
21 checks passed
@alongosz alongosz deleted the ibx-7911-drop-magic-getters branch June 5, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
7 participants