Generator matching optimization

[kirmorozov]

Description (*)

Changed implementation to be more transparent, and easy to read.
Plus using one strlen with explicit variable, rather than 3 implicit (via php internals for each strlen and minus operation).

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

1. php bin/magento setup:di:compile
2. ...

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)

Resolved issues:

1. resolves [Issue] Generator matching optimization #33809: Generator matching optimization

[m2-assistant]

Hi @kirmorozov. Thank you for your contribution
Here is 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

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, 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, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

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

[kirmorozov]

@magento run all tests

[kirmorozov]

@magento run Functional Tests B2B, Functional Tests CE

[gabrieldagama]

Hi @kirmorozov, thanks for contributing! Can you provide some extra information on execution time improvement? Ideally, we need to see the difference between your solution and vanilla Magento.

Can you provide that information?

Thanks!

[kirmorozov]

Thanks for asking, difference is visible at scale.

It all comes to strrposimplementation
vs substr_compare implementation.
strlenimplementation is almost free and taking string length value from zval.

strrpos will try to find last accuranse, instead of taking just tail.
substr_compare is equivalent of substr($search,-strlen($tail),strlen($tail)) === $tail, but does not make copy of a string to compare.

str_ends_withimplementation is even better in PHP8 because it does memory comparison instead of char-by-char.
https://www.php.net/manual/en/function.str-ends-with.php

Here is some opcode to read.

Magento\Framework\Code\Generator::generateClass: ; (lines=134, args=1, vars=11, tmps=53)
    ; (after optimizer)
    ; m2_core/lib/internal/Magento/Framework/Code/Generator.php:103-146
L0 (103):   EXT_NOP
L1 (103):   CV0($className) = RECV 1
L2 (105):   EXT_STMT
L3 (105):   ASSIGN CV1($resultEntityType) null
L4 (106):   EXT_STMT
L5 (106):   ASSIGN CV2($sourceClassName) null
L6 (107):   EXT_STMT
L7 (107):   T13 = FETCH_OBJ_R THIS string("_generatedEntities")
L8 (107):   V14 = FE_RESET_R T13 L50
L9 (107):   T15 = FE_FETCH_R V14 CV3($generatorClass) L50
L10 (107):  ASSIGN CV4($entityType) T15
L11 (108):  EXT_STMT
L12 (108):  INIT_NS_FCALL_BY_NAME 1 string("Magento\Framework\Code\ucfirst")
L13 (108):  SEND_VAR_EX CV4($entityType) 1
L14 (108):  V17 = DO_FCALL
L15 (108):  ASSIGN CV5($entitySuffix) V17
L16 (110):  EXT_STMT
L17 (110):  INIT_NS_FCALL_BY_NAME 2 string("Magento\Framework\Code\strrpos")
L18 (110):  SEND_VAR_EX CV0($className) 1
L19 (110):  SEND_VAR_EX CV5($entitySuffix) 2
L20 (110):  V19 = DO_FCALL
L21 (110):  INIT_NS_FCALL_BY_NAME 1 string("Magento\Framework\Code\strlen")
L22 (110):  SEND_VAR_EX CV0($className) 1
L23 (110):  V20 = DO_FCALL
L24 (110):  INIT_NS_FCALL_BY_NAME 1 string("Magento\Framework\Code\strlen")
L25 (110):  SEND_VAR_EX CV5($entitySuffix) 1
L26 (110):  V21 = DO_FCALL
L27 (110):  T22 = SUB V20 V21
L28 (110):  T23 = IS_IDENTICAL V19 T22
L29 (110):  JMPZ T23 L49
L30 (111):  EXT_STMT
L31 (111):  ASSIGN CV1($resultEntityType) CV4($entityType)
L32 (112):  EXT_STMT
L33 (112):  INIT_NS_FCALL_BY_NAME 2 string("Magento\Framework\Code\rtrim")
L34 (113):  INIT_NS_FCALL_BY_NAME 3 string("Magento\Framework\Code\substr")
L35 (113):  SEND_VAR_EX CV0($className) 1
L36 (113):  SEND_VAL_EX int(0) 2
L37 (113):  INIT_NS_FCALL_BY_NAME 1 string("Magento\Framework\Code\strlen")
L38 (113):  SEND_VAR_EX CV5($entitySuffix) 1
L39 (113):  V25 = DO_FCALL
L40 (113):  T26 = MUL V25 int(-1)
L41 (113):  SEND_VAL_EX T26 3
L42 (113):  V27 = DO_FCALL
L43 (113):  SEND_VAR_NO_REF_EX V27 1
L44 (114):  SEND_VAL_EX string("\") 2
L45 (114):  V28 = DO_FCALL
L46 (114):  ASSIGN CV2($sourceClassName) V28
L47 (116):  EXT_STMT
L48 (116):  JMP L50
L49 (107):  JMP L9
L50 (107):  FE_FREE V14
L51 (120):  EXT_STMT
L52 (120):  INIT_METHOD_CALL 3 THIS string("shouldSkipGeneration")

Magento\Framework\Code\Generator::generateClass: ; (lines=132, args=1, vars=12, tmps=52)
    ; (after optimizer)
    ; m2_core/lib/internal/Magento/Framework/Code/Generator.php:103-147
L0 (103):   EXT_NOP
L1 (103):   CV0($className) = RECV 1
L2 (105):   EXT_STMT
L3 (105):   ASSIGN CV1($resultEntityType) null
L4 (106):   EXT_STMT
L5 (106):   ASSIGN CV2($sourceClassName) null
L6 (107):   EXT_STMT
L7 (107):   T14 = FETCH_OBJ_R THIS string("_generatedEntities")
L8 (107):   V15 = FE_RESET_R T14 L48
L9 (107):   T16 = FE_FETCH_R V15 CV3($generatorClass) L48
L10 (107):  ASSIGN CV4($entityType) T16
L11 (108):  EXT_STMT
L12 (108):  INIT_NS_FCALL_BY_NAME 1 string("Magento\Framework\Code\strlen")
L13 (108):  SEND_VAR_EX CV4($entityType) 1
L14 (108):  V18 = DO_FCALL
L15 (108):  ASSIGN CV5($suffixLen) V18
L16 (109):  EXT_STMT
L17 (109):  INIT_NS_FCALL_BY_NAME 1 string("Magento\Framework\Code\ucfirst")
L18 (109):  SEND_VAR_EX CV4($entityType) 1
L19 (109):  V20 = DO_FCALL
L20 (109):  ASSIGN CV6($entitySuffix) V20
L21 (111):  EXT_STMT
L22 (111):  INIT_NS_FCALL_BY_NAME 4 string("Magento\Framework\Code\substr_compare")
L23 (111):  SEND_VAR_EX CV0($className) 1
L24 (111):  SEND_VAR_EX CV6($entitySuffix) 2
L25 (111):  T22 = MUL CV5($suffixLen) int(-1)
L26 (111):  SEND_VAL_EX T22 3
L27 (111):  SEND_VAR_EX CV5($suffixLen) 4
L28 (111):  V23 = DO_FCALL
L29 (111):  T24 = IS_EQUAL V23 int(0)
L30 (111):  JMPZ T24 L47
L31 (112):  EXT_STMT
L32 (112):  ASSIGN CV1($resultEntityType) CV4($entityType)
L33 (113):  EXT_STMT
L34 (113):  INIT_NS_FCALL_BY_NAME 2 string("Magento\Framework\Code\rtrim")
L35 (114):  INIT_NS_FCALL_BY_NAME 3 string("Magento\Framework\Code\substr")
L36 (114):  SEND_VAR_EX CV0($className) 1
L37 (114):  SEND_VAL_EX int(0) 2
L38 (114):  T26 = MUL CV5($suffixLen) int(-1)
L39 (114):  SEND_VAL_EX T26 3
L40 (114):  V27 = DO_FCALL
L41 (114):  SEND_VAR_NO_REF_EX V27 1
L42 (115):  SEND_VAL_EX string("\") 2
L43 (115):  V28 = DO_FCALL
L44 (115):  ASSIGN CV2($sourceClassName) V28
L45 (117):  EXT_STMT
L46 (117):  JMP L48
L47 (107):  JMP L9
L48 (107):  FE_FREE V15
L49 (121):  EXT_STMT
L50 (121):  INIT_METHOD_CALL 3 THIS string("shouldSkipGeneration")

Some benchmarks and ranks can be form with 3rdparty sources

[kirmorozov]

@magento run Functional Tests CE

[kirmorozov]

@gabrieldagama Do you think that switch to str_ends_with and polyfill is better?

[magento-engcom-team]

Hi @bgorski, thank you for the review.
ENGCOM-9121 has been created to process this Pull Request
✳️ @bgorski, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[hostep]

@engcom-Hotel: can you check the version of composer you are using to install the dependencies (composer --version)? I'm betting it's verion 1.x. I would advise you to upgrade it to version 2.x (composer selfupdate --2). That will probably solve your issue.

Sorry, I answered too soon before properly analysing the screenshot.

@kirmorozov: you'd probably best rebase your change on top of the latest 2.4-develop branch, so composer version 2 can be used to install dependencies.

OR

@engcom-Hotel could temporarily downgrade his composer version to version 1.x (composer selfupdate --1)

[bgorski]

@magento run all tests

[magento-automated-testing]

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.

[bgorski]

I merged the latest 2.4-develop to the PR branch myself to speed up the process a bit

[bgorski]

@magento run Functional Tests CE, Functional Tests EE, WebAPI Tests

[magento-automated-testing]

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.

[bgorski]

@engcom-Hotel I noticed you added the "Progress: needs update" tag here. May I know what changes are requested here? Were you able to re-test this?
Please let me know and put this back in the "Changed Requested" status if necessary. Thanks!

[engcom-Hotel]

Hello @bgorski,

As you mentioned in the comment, now no changes are required. I am resuming the testing of this PR.

Thank You

[engcom-Hotel]

✔️ QA Passed

Hello @kirmorozov,

Thanks for the contribution!

We have tested the PR. Please have a look into below description:

Manual testing scenario:

1. Deploy PR in the system.
2. Check the opcode of changes via the below command:
   php -d vld.active=1 -d vld.execute=0 -f ./lib/internal/Magento/Framework/Code/Generator.php
3. Compare the opcodes with existing Magento 2.4-develop
4. I am attaching both opcode files with this.

Before: ✖️
opcode_strrpos.log

After: ✔️
opcode_substr_compare.log

Tested all the manual scenarios, no impact on regression testing.

Thanks

[m2-assistant]

Hi @kirmorozov, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.