Improve include time of Mojo::Base by extracting monkey_patch

[okurz]

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.

[okurz]

• 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
• Removed the re-implementation of monkey_patch in Mojo::Util, directly re-exporting
• Replaced "use" by "require" as per https://github.com/mojolicious/mojo/pull/2173#discussion_r1553828119
• Fixed tests
• Added POD in Mojo::MonkeyPatch

[Grinnz]

• 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]

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

[okurz]

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

ok, do you have a preferred alternative? Move to yet another new module with a single function, combine with Mojo::MonkeyPatch in BaseUtils (???), re-implement the one-liner?

[okurz]

• 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]

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]

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.

[Grinnz]

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]

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

[poti1]

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

[poti1]

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.

[okurz]

• Updated POD for description
• Fixed one CI failure

[okurz]

• Using only strict+warnings in BaseUtils to address https://github.com/mojolicious/mojo/pull/2173#discussion_r1594074777

[kraih]

Please squash commits.

[okurz]

Please squash commits.

done

[okurz]

Covered all last three open review comments

[jhthorsen]

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]

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]

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]

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]

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]

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{$_} );
    }
}

[eduardoj]

Small typo in commit message and in pull request description: strictures shold be structures.

[okurz]

Small typo in commit message and in pull request description: strictures shold be structures.

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.

[kraih]

LGTM. don't know why perltidy test is failing

New perltidy release. Just rebase the PR on main and it will be fine.

[okurz]

Rebased

[Grinnz]

Thank you for persisting to tidy this up!