-
Notifications
You must be signed in to change notification settings - Fork 582
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 include time of Mojo::Base by extracting monkey_patch #2173
Conversation
c408463
to
949dbaa
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This should not be done. This creates a method in all classes using Mojo::Base as a base. |
Additionally, if Mojo::Util re-exports class_to_path no other adjustment should be necessary. |
This comment was marked as resolved.
This comment was marked as resolved.
done, updated |
BaseUtil seems reasonable and self-explanatory to me, but there may be other opinions. As long as Mojo::Util imports and re-exports the functions from there correctly, there should be no changes needed to other modules. |
Sure, let's see how that looks. Added a commit to combine monkey_patch and class_to_path in Mojo::BaseUtil and reverted changes in other modules to use still the functions as exported from Mojo::Util. |
IMO, t/mojo/util.t should remain unchanged, to ensure there is no change in compatibility for the re-exported functions. t/mojo/base_util.t may be unnecessary if so. |
I'd be interested in some real world benchmarks where this makes a difference. |
I'm looking forwards to seeing this hopefully soon in the library :) |
By simply copying over BEFORE: NOW: When running on my phone's termux, the change of those values is really felt. I compared before and after also with Nut Prof: BEFORE: AFTER: |
1942558
to
a8ba9a5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a8ba9a5
to
b7002c7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Please squash commits. |
b7002c7
to
597c50b
Compare
70f54e2
to
9299d31
Compare
9299d31
to
760a311
Compare
760a311
to
bfe6eb0
Compare
Small typo in commit message and in pull request description: |
bfe6eb0
to
f670d4f
Compare
uh, no. I meant strictures as from https://metacpan.org/pod/strictures but maybe that word only makes sense when using that pod. So rephrased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. don't know why perltidy test is failing
f670d4f
to
b27e4a4
Compare
New perltidy release. Just rebase the PR on main and it will be fine. |
Commit 7e9a2ad introduced a "require Mojo::Util" causing a significant chain of further dependencies being pulled in which IMHO should be avoided for the very base module which is in particular being advertised as useful for just enabling static code checks and common import checks. This commit moves out the function "monkey_patch" into its own module to break or prevent the circular dependency between Mojo::Base and Mojo::Util. With that the import of Mojo::Base is more efficient, `time perl -e 'use Mojo::Base` on my system reduced from 224±12.08 ms to 52.0±2.3 ms which I consider a considerable improvement for Mojo::Base which is used as a baseclass in many cases. Further minor changes included: * Directly require MonkeyPatch for cleaner subclassing + POD * Correct use of MonkeyPatch with empty import * Combine monkey_patch and class_to_path in new Mojo::BaseUtil
b27e4a4
to
9637ef0
Compare
Rebased |
Thank you for persisting to tidy this up! |
Motivation
Commit 7e9a2ad introduced a "require Mojo::Util" causing a significant chain of further dependencies being pulled in which IMHO should be avoided for the very base module which is in particular being advertised as useful for just enabling static code checks and common import checks.
Summary
This commit moves out the function "monkey_patch" into its own module to break or prevent the circular dependency between Mojo::Base and Mojo::Util.
With that the import of Mojo::Base is more efficient,
time perl -e 'use Mojo::Base
on my system reduced from 224±12.08 ms to 52.0±2.3 ms which I consider a considerable improvement for Mojo::Base which is used as a baseclass in many cases.