Improve include time of Mojo::Base by extracting monkey_patch #2173

okurz · 2024-04-05T12:37:41Z

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 strictures 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.

lib/Mojo/Util.pm

lib/Mojo/Base.pm

Grinnz · 2024-04-05T20:03:01Z

Addressed CI failure "Undefined subroutine &Mojo::Util::class_to_path" by moving class_to_path to Mojo::Base and adjusting the rest of the repository accordingly, re-exporting from Mojo::Util

This should not be done. This creates a method in all classes using Mojo::Base as a base.

Grinnz · 2024-04-05T20:04:37Z

Additionally, if Mojo::Util re-exports class_to_path no other adjustment should be necessary.

okurz · 2024-04-05T20:09:37Z

Addressed CI failure "Undefined subroutine &Mojo::Util::class_to_path" by moving class_to_path to Mojo::Base and adjusting the rest of the repository accordingly, re-exporting from Mojo::Util

This should not be done. This creates a method in all classes using Mojo::Base as a base.

done, updated

Grinnz · 2024-04-05T20:14:36Z

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.

okurz · 2024-04-05T20:34:35Z

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.

lib/Mojo/BaseUtil.pm

Grinnz · 2024-04-05T21:17:25Z

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.

kraih · 2024-04-05T23:55:46Z

I'd be interested in some real world benchmarks where this makes a difference.

poti1 · 2024-05-07T18:42:45Z

I'm looking forwards to seeing this hopefully soon in the library :)

poti1 · 2024-05-07T18:59:23Z

By simply copying over monkey_patch and the set_subname declarations from Mojo::Util to my cli tool (and removing use/require Mojo::Util), I saw these results in performance improvement:

BEFORE:
real 0m0.179s
user 0m0.101s
sys 0m0.052s

NOW:
real 0m0.045s
user 0m0.018s
sys 0m0.015s

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:
Profile of -e for 198ms (of 205ms), executing 15207 statements and 6061 subroutine calls in 76 source files and 27 string evals.

AFTER:
Profile of -e for 11.7ms (of 11.8ms), executing 144 statements and 57 subroutine calls in 8 source files.

lib/Mojo/BaseUtil.pm

kraih · 2024-05-08T16:21:51Z

Please squash commits.

okurz · 2024-05-08T17:20:19Z

Please squash commits.

done

lib/Mojo/BaseUtil.pm

t/mojo/util.t

lib/Mojo/Util.pm

okurz · 2024-05-08T18:54:45Z

Covered all last three open review comments

Grinnz

Looks good to me

jberger · 2024-05-08T21:57:03Z

lib/Mojo/Util.pm

 use Pod::Usage         qw(pod2usage);
 use Socket             qw(inet_pton AF_INET6 AF_INET);
-use Sub::Util          qw(set_subname);


Do you think anyone depends on Mojo::Util::set_subname? They probably shouldn't but they could

It isn't documented and the fix is trivial so I don't think I care at all.

I would not consider use of any subroutines imported without a documented interface as our responsibility.

No documentation, no feature.

jberger · 2024-05-08T21:58:25Z

t/mojo/base_util.t

+
+subtest 'monkey_patch' => sub {
+  {
+


nitpick but blank line?

ok, done. Included is a second commit removing redundant blank lines in the added code as well as where I originally copied the test code from.

Nope, perltidy actually demands the blank line :D Reverted the last change

jhthorsen · 2024-05-09T00:14:34Z

I think the name Mojo::BaseUtil is a bit awkward. How to tell what is in which package? I can't come up with a killer name, but I'm thinking about Mojo::CoreUtil or Mojo::PureUtil, which might indicate that the functions in that package is without any significant dependencies? I guess Mojo::BaseUtil makes sense, if we only have functions that Mojo::Base depends on, but I wonder if it's a slippery slope...

Should any of the following functions be moved over? The reason I ask, is because maybe it makes sense to move class_to_file(), when class_to_path() was moved.

camelize()
class_to_file()
decamelize()

Grinnz · 2024-05-09T00:19:04Z

While that might make sense for code level consistency, I think it's beneficial to keep the module limited in scope to its stated purpose. All of these functions should continue to be used from Mojo::Util in normal cases. It can be viewed less as "moving" the functions from the user's POV because they continue to be re-exported.

jhthorsen · 2024-05-09T00:31:32Z

Then I think that should be stated in the documentation, meaning that functions can be added and removed in the future to keep Mojo::Base quick to load.

Grinnz · 2024-05-09T00:38:30Z

It could make sense to me to remove the function documentation from the new module (these would need to be added as trusted in t/pod_coverage.t), and document that it provides functions for use in Mojo::Base and shouldn't be depended on to remain compatible.

okurz · 2024-05-09T16:58:24Z

It could make sense to me to remove the function documentation from the new module

ok, fine for me. As there was already one "+1" I followed that suggestion and included that change

Also included is a second commit removing redundant blank lines in the added code as well as where I originally copied the test code from.

poti1 · 2024-05-09T17:06:30Z

For my own cli script, I ended up doing this to avoid loading in thousands of subs right away:

sub monkey_patch {
    my ( $class, %patch ) = @_;
    require Sub::Util;
    for ( keys %patch ) {
        *{"${class}::$_"} =
          Sub::Util::set_subname( "${class}::$_", $patch{$_} );
    }
}

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 strictures 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

kraih · 2024-05-09T19:39:25Z

lib/Mojo/BaseUtil.pm

+
+our @EXPORT_OK = (qw(class_to_path monkey_patch));
+
+


Nitpick: Extra newline.

kraih · 2024-05-09T19:40:20Z

lib/Mojo/BaseUtil.pm

+L<Mojo::BaseUtil> provides a C<class_to_path> and the C<monkey_patch> function
+for L<Mojo>. The main purpose is to provide functions to both C<Mojo::Base>
+and C<Mojo::Util> so that C<Mojo::Base> does not have to load the rest of
+C<Mojo::Util>.


Nitpick: Not 120 char lines.

kraih · 2024-05-09T19:41:04Z

t/mojo/base_util.t

+use Mojo::BaseUtil qw(class_to_path monkey_patch);
+
+
+subtest 'class_to_path' => sub {


Nitpick: Extra newline.

kraih · 2024-05-09T19:41:31Z

t/pod_coverage.t

@@ -11,4 +11,7 @@ my @await = (
  qw(AWAIT_NEW_FAIL AWAIT_ON_CANCEL AWAIT_ON_READY AWAIT_WAIT)
 );

-all_pod_coverage_ok({also_private => ['BUILD_DYNAMIC', @await, 'spurt']});
+# base utils only to be used in Mojo::Base and should not be used elsewhere


Nitpick: Comment capitalisation.

Is that correct english? Sounds a little wrong to me.

guest20 reviewed Apr 5, 2024

View reviewed changes

lib/Mojo/Util.pm Outdated Show resolved Hide resolved

kraih added needs feedback feature request labels Apr 5, 2024

kraih reviewed Apr 5, 2024

View reviewed changes

lib/Mojo/Base.pm Outdated Show resolved Hide resolved

okurz force-pushed the feature/optimize_mojo_base branch from c408463 to 949dbaa Compare April 5, 2024 19:44

This comment was marked as resolved.

Sign in to view

Grinnz reviewed Apr 5, 2024

View reviewed changes

lib/Mojo/BaseUtil.pm Outdated Show resolved Hide resolved

Grinnz reviewed Apr 5, 2024

View reviewed changes

lib/Mojo/BaseUtil.pm Outdated Show resolved Hide resolved

Grinnz reviewed May 7, 2024

View reviewed changes

lib/Mojo/BaseUtil.pm Outdated Show resolved Hide resolved

okurz force-pushed the feature/optimize_mojo_base branch from 1942558 to a8ba9a5 Compare May 8, 2024 10:10

This comment was marked as outdated.

Sign in to view

kraih requested review from a team, marcusramberg, kraih, jhthorsen and jberger May 8, 2024 10:27

okurz force-pushed the feature/optimize_mojo_base branch from a8ba9a5 to b7002c7 Compare May 8, 2024 14:12

This comment was marked as outdated.

Sign in to view

okurz force-pushed the feature/optimize_mojo_base branch from b7002c7 to 597c50b Compare May 8, 2024 17:20

kraih reviewed May 8, 2024

View reviewed changes

lib/Mojo/BaseUtil.pm Outdated Show resolved Hide resolved

Grinnz reviewed May 8, 2024

View reviewed changes

t/mojo/util.t Outdated Show resolved Hide resolved

Grinnz reviewed May 8, 2024

View reviewed changes

lib/Mojo/Util.pm Outdated Show resolved Hide resolved

okurz force-pushed the feature/optimize_mojo_base branch from 597c50b to b200779 Compare May 8, 2024 18:54

Grinnz previously approved these changes May 8, 2024

View reviewed changes

jberger reviewed May 8, 2024

View reviewed changes

okurz dismissed Grinnz’s stale review via 3fa30df May 9, 2024 16:57

okurz force-pushed the feature/optimize_mojo_base branch from 3fa30df to ef789ba Compare May 9, 2024 17:14

kraih reviewed May 9, 2024

View reviewed changes

lib/Mojo/BaseUtil.pm

our @EXPORT_OK = (qw(class_to_path monkey_patch));

Copy link

Member

kraih May 9, 2024

Choose a reason for hiding this comment

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

Nitpick: Extra newline.

kraih reviewed May 9, 2024

View reviewed changes

t/mojo/base_util.t

use Mojo::BaseUtil qw(class_to_path monkey_patch);

subtest 'class_to_path' => sub {

Copy link

Member

kraih May 9, 2024

Choose a reason for hiding this comment

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

Nitpick: Extra newline.

kraih reviewed May 9, 2024

View reviewed changes

		use Mojo::BaseUtil qw(class_to_path monkey_patch);


		subtest 'class_to_path' => sub {

Improve include time of Mojo::Base by extracting monkey_patch #2173

Are you sure you want to change the base?

Improve include time of Mojo::Base by extracting monkey_patch #2173

Conversation

okurz commented Apr 5, 2024

Motivation

Summary

This comment was marked as resolved.

Grinnz commented Apr 5, 2024

Grinnz commented Apr 5, 2024

This comment was marked as resolved.

okurz commented Apr 5, 2024

Grinnz commented Apr 5, 2024

okurz commented Apr 5, 2024

Grinnz commented Apr 5, 2024

kraih commented Apr 5, 2024

poti1 commented May 7, 2024

poti1 commented May 7, 2024 • edited

This comment was marked as outdated.

This comment was marked as outdated.

kraih commented May 8, 2024

okurz commented May 8, 2024

okurz commented May 8, 2024

Grinnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhthorsen commented May 9, 2024

Grinnz commented May 9, 2024

jhthorsen commented May 9, 2024

Grinnz commented May 9, 2024

okurz commented May 9, 2024

poti1 commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poti1 commented May 7, 2024 •

edited