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.

Sign up for GitHub

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

Jump to bottom

Autoloader: Remove file check to improve performance. #15118

Merged
merged 1 commit into from
Mar 26, 2020
Merged

Autoloader: Remove file check to improve performance. #15118

merged 1 commit into from
Mar 26, 2020

Conversation

jeffstieler
Copy link
Contributor

https://github.com/Automattic/jetpack/blob/master/packages/autoloader/src/AutoloadGenerator.php#L68

Since we always optimize, remove expensive file_exists() calls from autoloader.

The current autoloader essentially enforces Composer's Optimization Level 1.

This PR seeks to enforce Optimization Level 2 as well.

In testing WooCommerce + WooCommerce Admin (which both use this autoloader), requests were seeing ~375 calls to file_exists() and spending 1+ seconds wall time. With this change, those numbers are down to ~90 calls and ~0.2 seconds.

I see that a refactor is underway.. this PR maps to this line in the refactor: https://github.com/Automattic/jetpack/pull/15106/files#diff-354912567a0f128335ec5caad8b6e3f8R124

Changes proposed in this Pull Request:

  • Stop checking file_exists() before require_once Jetpack autoloader enqueued classes.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Tweaking the autoloader logic

Testing instructions:

I could use some help defining testing instructions for this PR.

Locally, I manually made these changes to the autoloaders in both WooCommerce and WooCommerce Admin in order to verify the performance improvements.

Proposed changelog entry for your changes:

  • Improve performance of autoloader.

@jeffstieler jeffstieler added [Focus] Performance [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Focus] Jetpack DNA labels Mar 25, 2020
@jeffstieler jeffstieler requested a review from a team March 25, 2020 17:45
@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Mar 25, 2020
@jeherve jeherve added this to the 8.4 milestone Mar 25, 2020
@jeherve jeherve requested review from kbrown9 and zinigor March 25, 2020 17:47
@jeherve jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Mar 25, 2020
@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 894221d

kbrown9
kbrown9 approved these changes Mar 25, 2020
View reviewed changes
Copy link
Member

@kbrown9 kbrown9 left a comment

Choose a reason for hiding this comment

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

This looks good to me! I performed a quick test of the PR by using the updated Autoloader with Jetpack. As expected, no problems occurred.

I'll leave this open for a day in case anyone else wants to take a look at it before merging. When it's merged, we'll make sure this change is included in the new autoloader feature branch that's currently in progress.

@jeffstieler
Copy link
Contributor Author

Thanks for the quick review @kbrown9!

zinigor
zinigor approved these changes Mar 25, 2020
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 26, 2020
jeherve
jeherve approved these changes Mar 26, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Tests are all good for this PR. Merging.
https://travis-ci.org/github/Automattic/jetpack/builds/666903637

@jeherve jeherve merged commit 348a583 into master Mar 26, 2020
@jeherve jeherve deleted the remove/autoloader-file-exists branch March 26, 2020 07:57
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 26, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
Changelog: add #15118
17db014
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
@nerrad nerrad mentioned this pull request Apr 2, 2020
Closed
@sergeymitr sergeymitr mentioned this pull request Apr 8, 2020
Merged
@kbrown9 kbrown9 mentioned this pull request Jun 10, 2020
Merged
@jeherve jeherve removed the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbrown9 kbrown9 kbrown9 approved these changes

@jeherve jeherve jeherve approved these changes

@zinigor zinigor zinigor approved these changes

Assignees

@jeffstieler jeffstieler

Labels
[Focus] Jetpack DNA [Focus] Performance [Package] Autoloader [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.4
Development

Successfully merging this pull request may close these issues.
6 participants
@jeffstieler @jetpackbot @zinigor @jeherve @kbrown9 @matticbot