Subject: Re: [PATCH v5 10/13] send-email: lazily load modules for a big speedup

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sat, 29 May 2021 10:12:53 +0200

On Fri, May 28 2021, Felipe Contreras wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> Optimize the time git-send-email takes to do even the simplest of
>> things (such as serving up "-h") from around ~150ms to ~80ms-~90ms by
>> lazily loading the modules it requires.
>> 
>> Before this change Devel::TraceUse would report 99/97 used modules
>> under NO_GETTEXT=[|Y], respectively. Now it's 52/37. It now takes ~15s
>> to run t9001-send-email.sh, down from ~20s.
>> 
>> Changing File::Spec::Functions::{catdir,catfile} to invoking class
>> methods on File::Spec itself is idiomatic. See [1] for a more
>> elaborate explanation, the resulting code behaves the same way, just
>> without the now-pointless function wrapper.
>
> I would reference `man File::Spec` rather than an email.
>
> And while this change makes sense, I think it should be split in two.
>
> Instead of doing:
>
>   -use Term::ANSIColor;
>   -print color("reset"), "\n";
>   +require Term::ANSIColor;
>   +print Term::ANSIColor::color("reset"), "\n";
>
> We could do this in one patch:
>
>   -print color("reset"), "\n";
>   +print Term::ANSIColor::color("reset"), "\n";
>
> That is just no-op noise that we can mostly ignore in the review.
>
> Then the actual change to require Term::ANSIColor selectively would be
> much simpler to see.

You mean do the change from imported functions in one commit, and then
sprinkle the "require" in another one?

I think it's clearer this way, you can't really assert that it worked as
intended (i.e. you have no more imports) without the s/use/require/g, so
it makes sense to do both as one atomic change.