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-1467: Fixed OOM regression during Subtree copying #3128

Merged
merged 2 commits into from
Nov 30, 2021

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Nov 17, 2021

Question Answer
JIRA issue IBX-1467
Type bug
Target eZ Platform version v2.5
BC breaks no

Aliases should be now unique which would fix the OOM problems.

Original PR: #3127

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

@barw4 barw4 requested a review from adamwojs November 22, 2021 11:56
@Steveb-p
Copy link
Contributor

Steveb-p commented Nov 24, 2021

@barw4 could you explain why the array receives duplicate entries in the first place?

From what I see, if a duplicate identifier is detected, the whole foreach loop is skipped.

EDIT: Ok, I've reviewed the code that actually does the merger and I understand now why it happens.

I'd propose to switch to a different approach, which will also be quite a bit faster in terms of checking that identifier exists in the array: let's use hashmap instead.

Currently, $alreadyGeneratedAliases is an array of strings (array<int, string>). If we change it to an associative array with identifiers as keys (array<string, true>), then you could use isset($alreadyGeneratedAliases[$identifier] to check that an entry already exists (instead of in_array) and push a value into $alreadyGeneratedAliases by using $alreadyGeneratedAliases[$identifier] = true.

This will change the complexity of check from o(n) to o(1) and allow array_merge to work properly.

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.

My proposal is in a separate comment.

@barw4 barw4 requested a review from Steveb-p November 25, 2021 15:12
@Steveb-p
Copy link
Contributor

@ViniTou substantial change to the original solution, hence re-request for review.

Copy link

@tomaszszopinski tomaszszopinski 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 eZPlatform 2.5.

@Steveb-p Steveb-p merged commit 17a60f8 into 7.5 Nov 30, 2021
@Steveb-p Steveb-p deleted the ibx-1467-oom-regression-subtree-copy branch November 30, 2021 12:32
@barw4
Copy link
Member Author

barw4 commented Nov 30, 2021

Merged into:
1.3: ezsystems/ezplatform-kernel@104098a
ibexa/core/main: ibexa/core#27

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

Successfully merging this pull request may close these issues.

6 participants