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-5388: Fixed performance issues of content updates after field changes #397

Merged
merged 34 commits into from
May 10, 2024

Conversation

Nattfarinn
Copy link
Contributor

@Nattfarinn Nattfarinn commented Dec 14, 2023

Question Answer
JIRA issue IBX-5388
Type improvement
Target Ibexa version v3.3
BC breaks no (or soft)

When new field was added we iterated over each content and applied field changes with default values to each existing version. When we removed the field we iterated over each content and removed data from every version as well. This cause major performance issue as all these changes happened in runtime each time you tried to publish Content Type and grow exponentially with increasing number of content items leading to memory issues, transaction nesting issues and exceeded execution times.

This PR introduces two keystones:

  • on field removed: there is no need to update content items, we can just ignore additional data within persistence layer during content load,
  • on field added: there is no need to update content items with default field values, as we can add these fields in runtime to object without touching persistence (like "virtual" fields).

This makes publishing Content Type process much faster and doesn't make content loading process slower (loading times differences are unnoticeable). Change itself is almost transparent to the user and developer and these virtual fields does not need any special handling for day-to-day operations. Once the content is published, copied or anyhow saved, virtual fields become persisted and will be indistinguishable from any other field. But there are some behavior changes, but with low impact:

  • virtual fields are not stored: this means fields do not have an id (it remains null),
  • virtual fields are dynamic: this means default value is not static anymore. Changes to default values within Content Type field definitions will be immediately reflected by all content items that still rely on virtual fields.

Depends on: https://github.com/ezsystems/ezplatform-version-comparison/pull/85

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/engineering-team).

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

The direction here makes perfect sense. We need to test this extensively of course:

Code-wise remarks:

eZ/Publish/Core/Persistence/Legacy/Content/Mapper.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/Content/Mapper.php Outdated Show resolved Hide resolved
eZ/Publish/SPI/Tests/FieldType/BaseIntegrationTest.php Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Dec 20, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.5% Duplication on New Code

See analysis details on SonarCloud

@alongosz alongosz added the Doc needed The changes require some documentation label Dec 21, 2023
@Nattfarinn Nattfarinn requested a review from a team December 21, 2023 10:33
alongosz
alongosz previously approved these changes Dec 21, 2023
@alongosz alongosz requested a review from a team December 21, 2023 11:04
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

LGTM. All my comments are optional notes that you can ignore as you see fit, since they are mostly opinion-based.

One thing I would very much like to add, but would require effort (probably a lot), is an integration test for it. It would be extremely valuable in my opinion, as we are dealing with a behavior that is very specific and depends on interaction between multiple components.

@micszo micszo self-assigned this Feb 28, 2024
Copy link
Member

@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.

Following issue found in image asset.

Steps:

  1. image CT (default), image asset CT (ezstring, ezimageasset)
  2. create image
  3. create image asset, select from repository
  4. edit image CT, add single relation field or keywords
  5. edit image asset
  6. open image editor
  7. flip image
  8. save as / apply changes to all

Actual result: In UI error occurs "Internal sever error". Exception occurs in log:

php.CRITICAL: Uncaught Error: reset(): Argument #1 ($array) must be of type array, null given {"exception":"[object] (TypeError(code: 0): reset(): Argument #1 ($array) must be of type array, null given at /Users/michalszoltysek/Projects/workspace/ibexa_website_4/vendor/ezsystems/ezplatform-kernel/eZ/Publish/Core/FieldType/Relation/SearchField.php:32)"} []

or

php.CRITICAL: Uncaught Error: implode(): Argument #1 ($pieces) must be of type array, string given {"exception":"[object] (TypeError(code: 0): implode(): Argument #1 ($pieces) must be of type array, string given at /Users/michalszoltysek/Projects/workspace/ibexa_website_4/vendor/ezsystems/ezplatform-kernel/eZ/Publish/Core/FieldType/Keyword/SearchField.php:29)"} []

@Nattfarinn Nattfarinn requested a review from micszo March 21, 2024 11:07
Copy link
Member

@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.

Retest status.

  • ezobjectrelation: OK
  • ezkeyword: NOK - the same exception.

@pspanja
Copy link
Contributor

pspanja commented Apr 8, 2024

For what it's worth, there was a way to process Content update asynchronously, removed as deprecated in the PR: ezsystems/ezpublish-kernel#2976

IMHO, this should be properly handled by asynchronous processing and not by introducing inconsistencies to the Repository API.

@micszo micszo self-requested a review April 10, 2024 12:53
Copy link
Member

@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.

QA Approved on Ibexa Commerce 3.3.38-dev.

@micszo micszo removed their assignment Apr 18, 2024
@Nattfarinn Nattfarinn requested a review from micszo April 23, 2024 07:41
@Nattfarinn Nattfarinn force-pushed the ibx-5388-content-type-performance branch from d881202 to 0f7966d Compare April 23, 2024 11:38
Copy link

sonarcloud bot commented Apr 25, 2024

Quality Gate Passed Quality Gate passed

Issues
6 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@alongosz alongosz merged commit 98b7b50 into 1.3 May 10, 2024
23 checks passed
@alongosz alongosz deleted the ibx-5388-content-type-performance branch May 10, 2024 13:08
Nattfarinn added a commit to ibexa/core that referenced this pull request May 29, 2024
…nges

For more details see https://issues.ibexa.co/browse/IBX-5388 and ezsystems/ezplatform-kernel#397

Key changes:

* Fixed performance issues of content updates after field definition changes

* Made DefaultDataFieldStorage extend FieldStorage

* [Tests] Aligned test coverage with the changes

---------

Co-Authored-By: Paweł Niedzielski <Steveb-p@users.noreply.github.com>
Co-Authored-By: Andrew Longosz <alongosz@users.noreply.github.com>
alongosz added a commit to ibexa/core that referenced this pull request Jun 5, 2024
This is an up-merge of ezsystems/ezplatform-kernel#397. See also #376 for more details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc needed The changes require some documentation Ready for review
Development

Successfully merging this pull request may close these issues.

9 participants