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

Improve Directory tree performance of New Media Gallery #35404

Conversation

nazarklovanych
Copy link

REOPENED OLD PR -> #30629

Description (*)

Scan only directories for directory tree component
The result of using glob instead of iterator with more than 500000 files in media folder

2020-10-23-225235_1920x1080_scrot

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Magento 2.4.1 Media Gallery....not ready for prime time. Fails when you have large amounts of data (images) #30596

Manual testing scenarios (*)

  1. A lot of images in filesystem -> execute this file to generate empty txt files in media folder https://gist.github.com/Nazar65/61c756ce76afa8c6718bf670794d548c execute in CMD bash glob.sh
  2. Set memory_limit in php to 100MB
  3. Open Media Galley
  4. Media gallery directory tree component renders directory tree component

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented May 2, 2022

Hi @nazarklovanych. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@m2-community-project m2-community-project bot added Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Severity: S1 Affects critical data or functionality and forces users to employ a workaround. labels May 2, 2022
@nazarklovanych nazarklovanych force-pushed the directory-tree-performance branch from 68f5dff to a43b3b9 Compare May 2, 2022 09:01
@nazarklovanych
Copy link
Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@nazarklovanych nazarklovanych force-pushed the directory-tree-performance branch from a43b3b9 to b419cdc Compare May 2, 2022 17:43
@nazarklovanych
Copy link
Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@nazarklovanych nazarklovanych force-pushed the directory-tree-performance branch from b419cdc to b2700f1 Compare May 3, 2022 08:33
@nazarklovanych
Copy link
Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@nazarklovanych nazarklovanych changed the title Improve Directory tree performance of New Media Gallery [WIP]Improve Directory tree performance of New Media Gallery May 3, 2022
@nazarklovanych nazarklovanych force-pushed the directory-tree-performance branch from b2700f1 to 8b9d7c4 Compare May 4, 2022 21:21
@nazarklovanych
Copy link
Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@nazarklovanych nazarklovanych changed the title [WIP]Improve Directory tree performance of New Media Gallery Improve Directory tree performance of New Media Gallery May 4, 2022
@nazarklovanych nazarklovanych force-pushed the directory-tree-performance branch from 8b9d7c4 to 32359e8 Compare May 4, 2022 21:44
@nazarklovanych
Copy link
Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@nazarklovanych
Copy link
Author

@magento run Integration Tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@nazarklovanych nazarklovanych force-pushed the directory-tree-performance branch from 32359e8 to 25a7cdc Compare May 5, 2022 07:02
@nazarklovanych
Copy link
Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@nazarklovanych nazarklovanych force-pushed the directory-tree-performance branch from 25a7cdc to 7b86ac0 Compare May 5, 2022 07:31
@nazarklovanych
Copy link
Author

@magento run all tests

@kimurabanyan
Copy link

@perfpcs
I commented because I found a performance issue in this PR code. It seems to be related to the problem you have.
I haven't suggested a fix for this issue. I'm not currently using Adobe Stock, so I'm using the old media gallery to work around this issue.

@perfpcs
Copy link

perfpcs commented Jul 12, 2022

@kimurabanyan OK I understand you. Thanks for clarifying. I guess Magento has no one on their team that can fix this properly because most everyone I know all reverted back to old media gallery because the new one is non-functional. Sad!

@interwijs
Copy link

@perfpcs, when using the old media gallery I also experience problems in Magento 2.4.4. Images are not visible and the directory tree only shows Storage Root directory with 2 empty subdirectories with no name. When I try to upload an image I get the message: We can't upload the file to the current folder right now. Please try another folder. And when I try to add a directory I get the message: We cannot create the folder under the selected directory. So the old media gallery is also useless right now.

@perfpcs
Copy link

perfpcs commented Jul 12, 2022

@interwijs I have not yet reverted to old media gallery in 2.4.4 but I will try it and see if it also is broke. Seems hard to believe if this is so that there is not more uproar from the community with no workable media gallery. And after 1.5 years of them knowing the issues, still no viable fix. Yet they are worried about merging code that does not even fix the problem. All this is hard to understand.

@pmonosolo
Copy link

Hello,

I tried this code and found the problem. First of all, the pattern used in "isPathExcluded->execute" would be as follows if it is Magento default.

 /^(wysiwyg\b(?!-)(?:\/?[^\/]+)*\/?$|catalog[\/]+category\b(?!-)(?:\/?[^\/]+)*\/?$)/

If $relativePath ='catalog', this pattern does not match, so getDirectories is recursively called in the following if statement.

https://github.com/magento/magento2/pull/35404/files#diff-f9876dbb5d6835b068311c3353b9ea2d0b535c7069bf474429822a3db8c960e9R81

The problem here is that under "catalog" there is not only a "category" but also a "product" directory. In the recursively called getDirectories, $relativePath = 'catalog/category' will not be in the same if statement, but 'catalog/product' will be in the if statement and getDirectories will be called again. In other words, getDirectories will be executed in all directories under 'catalog/product'.

This has a huge impact, and the more products we have, the more impact we will have.

UGH...it looks like this is the culprit - this folder needs to be blocked off from being searched as there's no need to access it - its completely automated by the product creation / import processes.

@sprankhub
Copy link
Member

This PR fixed the issues for us. Thanks, @nazarklovanych!

@perfpcs
Copy link

perfpcs commented Jan 18, 2023

Still broken for me on 2.4.5-p1

#30596

@sprankhub
Copy link
Member

@perfpcs, yes, it is broken in 2.4.5-p1. However, did you try to apply the changes from this PR manually? Did you check if the changes from this PR solve the issue for you?

@perfpcs
Copy link

perfpcs commented Jan 20, 2023

@sprankhub , yes I tried and am using this PR but it only partially works. Editing directory.xml results in no changes to the tree whatsoever so we can never get ONLY the WYSIWYG directory. Also we are missing many of the things that make this useful like the search box, sorting drop downs, etc.

image

Do you have all these things showing?

@sprankhub
Copy link
Member

yes I tried and am using this PR but it only partially works.

Please mind that this PR is about the performance / memory issue with large media directories. If you have different issues, you might want to look into other / open new issues.

Editing directory.xml results in no changes to the tree whatsoever so we can never get ONLY the WYSIWYG directory.

This is not part of this PR.

Also we are missing many of the things that make this useful like the search box, sorting drop downs, etc.

I do see all this:

image

@engcom-Hotel
Copy link
Contributor

Hello @nazarklovanych,

First of all, thank you for your contribution and collaboration!

We are in the process of reviewing this PR, but it seems there are some conflicts. So we request you to please resolve the conflicts so that we move further with this PR.

Meanwhile, we are moving this PR On Hold, we will pick this up when the conflicts resolve.

Thanks

@engcom-Hotel
Copy link
Contributor

Hello @nazarklovanych,

Have you got a chance to resolve the conflicts of this PR?

Thanks

@engcom-Hotel
Copy link
Contributor

Hi @nazarklovanych,

Thank you for your contribution!

As per the comment here #35404 (comment) we are unable to move further on this PR due to conflicts. Can you please look into it?

Till then we are closing this PR.

Please feel free to reopen or let us know when you are ready to work on it again, and we will be happy to re-open it.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Project: Community Picked PRs upvoted by the community Severity: S1 Affects critical data or functionality and forces users to employ a workaround.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Magento 2.4.1 Media Gallery....not ready for prime time. Fails when you have large amounts of data (images)
9 participants