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

MSI-2522: Introduce yielding for order list iterations and use paging #2573

Closed

Conversation

vadimjustus
Copy link
Contributor

@vadimjustus vadimjustus commented Sep 21, 2019

Introduce yielding for order iterations to improve memory usage.

Description (*)

Extends GetOrdersInFinalState and GetOrdersInNotFinalState classes in order to return \Traversable while using yield.

Test results with 1255 orders:

  • Without changes from this PR: 115.10 seconds / 1308.25 MB memory usage
  • With changes from this PR: 117.81 seconds / 684.25 MB memory usage

Fixed Issues (if relevant)

  1. Inventory reservation list inconsistencies command does not work #2522: Inventory reservation list inconsistencies command does not work

Manual testing scenarios (*)

  1. Create a fixtures with 1000+ orders
  2. Checkout to 2.3-develop branch
  3. Run bin/magento inventory:reservation:list-inconsistencies
  4. Monitor memory usage
  5. Checkout to MSI-2522-consistency-cli-mem-usage-improvements branch
  6. Run bin/magento inventory:reservation:list-inconsistencies
  7. Monitor memory usage

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)

@vadimjustus vadimjustus changed the title MSI-2522: Introduce yielding for order list iterations and use paging WIP: MSI-2522: Introduce yielding for order list iterations and use paging Sep 21, 2019
@vadimjustus vadimjustus changed the title WIP: MSI-2522: Introduce yielding for order list iterations and use paging MSI-2522: Introduce yielding for order list iterations and use paging Sep 22, 2019
*/
public function getOrderIncrementId(): string
{
return (string)$this->orderIncrementId;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need cast here

@@ -25,7 +25,7 @@ public function execute(array $inconsistencies): array
return array_filter(
$inconsistencies,
function (SalableQuantityInconsistency $inconsistency) {
return (bool)$inconsistency->getOrder();
return $inconsistency->hasAssignedOrder() !== false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please get rid of !== false

yield $item->getEntityId() => $item;
}

gc_collect_cycles();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need it?

@ishakhsuvarov
Copy link
Contributor

Closing, as #2578 includes these changes

@ishakhsuvarov ishakhsuvarov deleted the MSI-2522-consistency-cli-mem-usage-improvements branch December 5, 2019 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants