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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 29, 2021, 14:24 UTC
Message-ID: <60b24e84658ff_2650882081a@natae.notmuch>
In-Reply-To: <87im322c04.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason wrote:

Show 40 quoted lines

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

Yes.

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

It is clear, but there's some noise.

And I don't see how you can assert there are no more imports from this patch, especially when the next patch gets rid of more imports.

Moreover, it's already the case that the code uses namespaces in some cases: Net::Domain::domainname is one you did not have to convert. You would be doing the same for all other instances.

Cheers.

-- 
Felipe Contreras