Improve new empty item creation by using clone when possible #36369
Conversation
This patch improves creation time of new objects by using clone whenever possible. I'm still testing this, but would like to get feedback on this:
Old version with PLP with all caches enabled:
COUNTER:129
Timer:17.240343 ms
Old version with PLP empty caches:
COUNTER:4470
Timer: 381.315609 ms
Old version with PLP no block cache:
COUNTER:2398
Timer:202.855686 ms
New version with PLP no block cache:
COUNTER:2398
Timer: 2.540631 ms
Old version Category Admin page:
COUNTER:5919
Timer: 1423.777139 ms
New version Category Admin page:
COUNTER:5919
Timer: 8.040596ms
|
Hi @igorwulff. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
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. 🕙 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 |
|
@igorwulff I don't think it's the right thing since clone might cause copying data from an existing object to new one, while earlier, it was always creating a new one. |
ihor-sviziev
added
Priority: P2
You are right. I will think about possible alternative ways to see if and how this can be done. |
Update collection for a local instance cache which we can use to clone new empty objects.
| if ($this->newEmptyItem !== null | ||
| && $this->_itemObjectClass !== $className | ||
| ) { | ||
| $this->newEmptyItem === null; | ||
| } |
There was a problem hiding this comment.
I think it's redundant code because this is mean that one collection could have multiple items with different classes.
There was a problem hiding this comment.
@Den4ik I've removed the redundant code.
m2-community-project
bot
added
Progress: review
and removed
Progress: pending review
labels
Revert changes to setItemObjectClass method and removed the additional checks in there that should not be needed.
m2-community-project
bot
added
Progress: review
and removed
Progress: needs update
labels
|
@magento run all tests |
|
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. |
|
@magento run all tests |
|
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. |
|
@magento run WebAPI Tests |
|
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. |
|
Hi @Den4ik can you clarify for me if the issues in the tests are related/caused by the changes in this PR? |
Some object utilize xml properies to load more data and parameters into them, like the indexers through the constructor. Checking if the empty item is just an instanceof \Magento\Framework\Model\AbstractModel might be enough to circumvent this potential problem.
|
@magento run all tests |
|
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. |
m2-community-project
bot
added
Progress: pending review
and removed
Progress: review
labels
This patch improves creation time of new objects by using clone whenever possible. I'm still testing this, but would like to get feedback on this. I've been adding a timer that compared the old and new result in the same transaction, when navigating through the site there are some gains to be found. Especially when cached are more cold (like block_html), admin areas like category view show the biggest gains:
I can track per transaction type, these are just logged results for now from across frontend and admin area:
TIMER:-19.572875 ms COUNTER: 2741
TIMER:-1.616937 ms COUNTER: 16
TIMER:-7.571272 ms COUNTER: 1506
TIMER:0 ms COUNTER: 0
TIMER:-1.572855 ms COUNTER: 16
TIMER:-23.672388 ms COUNTER: 801
TIMER:-3.223335 ms COUNTER: 11
TIMER:-13.592401 ms COUNTER: 75
TIMER:-2.116431 ms COUNTER: 11
TIMER:-7.662507 ms COUNTER: 74
TIMER:-4.198262 ms COUNTER: 49
TIMER:-72.5475 ms COUNTER: 5855
TIMER:-4.133812 ms COUNTER: 11
TIMER:-4.779091 ms COUNTER: 49
TIMER:-2.301819 ms COUNTER: 11
TIMER:-8.377065 ms COUNTER: 813
TIMER:-6.833618 ms COUNTER: 813
TIMER:-2.434347 ms COUNTER: 11
TIMER:-3.818474 ms COUNTER: 49
TIMER:-10.169898 ms COUNTER: 814
TIMER:-3.050243 ms COUNTER: 11
TIMER:-3.200632 ms COUNTER: 49
TIMER:-5.976394 ms COUNTER: 813
TIMER:-2.748932 ms COUNTER: 11
TIMER:-3.96872 ms COUNTER: 49
TIMER:-106.360966 ms COUNTER: 5855
TIMER:-3.895319 ms COUNTER: 11
TIMER:-3.334799 ms COUNTER: 49
TIMER:-2.288225 ms COUNTER: 11
TIMER:-60.319299 ms COUNTER: 5855
TIMER:-3.454251 ms COUNTER: 11
TIMER:-3.306505 ms COUNTER: 49
TIMER:-2.563021 ms COUNTER: 11
TIMER:-88.340151 ms COUNTER: 5856
TIMER:-3.875317 ms COUNTER: 11
TIMER:-4.289538 ms COUNTER: 49
TIMER:-2.396556 ms COUNTER: 11
TIMER:-8.741497 ms COUNTER: 813
TIMER:-2.273909 ms COUNTER: 11
TIMER:-3.113306 ms COUNTER: 49
TIMER:-9.474795 ms COUNTER: 674
TIMER:-4.063281 ms COUNTER: 49
Description (*)
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)