GNU bug report logs - #44529
[PATCH] Convert apropos-internal from C to Lisp

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 44529 in the body.
You can then email your comments to 44529 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Convert apropos-internal from C to Lisp
Date: Mon, 9 Nov 2020 02:38:59 -0800

[Message part 1 (text/plain, inline)]

Severity: wishlist

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> I discovered that apropos-internal is defined in keymap.c for a reason
> that escapes me (it really should be moved to apropos.el).

Yes.  So I was actually going to propose the attached patch.  In my
testing, it is as fast as the C version (especially when byte-compiled),
which would match my intuition from reading the code.

(benchmark-run 10
  (apropos-command "test"))
=> (0.12032415399999999 2 0.014772391999999995) ; C
=> (0.13513192100000002 2 0.017216643000000004) ; Lisp

(benchmark-run 10
  (apropos "x"))
=> (3.846117816 131 1.092690677) ; C
=> (4.218219444 145 1.2153865740000003) ; Lisp

Any comments or objections?

[0001-Convert-apropos-internal-from-C-to-Lisp.patch (text/x-diff, attachment)]

Message #8 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 44529 <at> debbugs.gnu.org
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Mon, 09 Nov 2020 08:51:28 -0500

> Any comments or objections?

LGTM,


        Stefan

Message #11 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 44529 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Mon, 09 Nov 2020 18:02:00 +0200

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Mon, 9 Nov 2020 02:38:59 -0800
> Cc: monnier <at> iro.umontreal.ca
> 
> > I discovered that apropos-internal is defined in keymap.c for a reason
> > that escapes me (it really should be moved to apropos.el).
> 
> Yes.  So I was actually going to propose the attached patch.  In my
> testing, it is as fast as the C version (especially when byte-compiled),
> which would match my intuition from reading the code.
> 
> (benchmark-run 10
>   (apropos-command "test"))
> => (0.12032415399999999 2 0.014772391999999995) ; C
> => (0.13513192100000002 2 0.017216643000000004) ; Lisp
> 
> (benchmark-run 10
>   (apropos "x"))
> => (3.846117816 131 1.092690677) ; C
> => (4.218219444 145 1.2153865740000003) ; Lisp

20% slowdown is not negligible, at least not in principle.  Let's wait
for more people to voice their opinions on this aspect.

> Any comments or objections?

A bother: apropos.el is not preloaded, so please double-check that
none of the preloaded files call apropos-internal, otherwise that
would mean we need to preload apropos.el, which I think is
undesirable.

Thanks.

Message #14 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 44529 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Mon, 9 Nov 2020 12:44:26 -0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> (benchmark-run 10
>>   (apropos "x"))
>> => (3.846117816 131 1.092690677) ; C
>> => (4.218219444 145 1.2153865740000003) ; Lisp
>
> 20% slowdown is not negligible, at least not in principle.  Let's wait
> for more people to voice their opinions on this aspect.

Unless I'm missing something, the slowdown should be around 10%.

(FWIW, the figure of 0.38 or 0.42 seconds is for the more unlikely
string with one character.  In the more realistic scenario with a
slightly longer string, we see a difference between 0.012 and 0.013
seconds.)

> A bother: apropos.el is not preloaded, so please double-check that
> none of the preloaded files call apropos-internal, otherwise that
> would mean we need to preload apropos.el, which I think is
> undesirable.

I only see one call in a preloaded file:

./lisp/progmodes/elisp-mode.el:873: (dolist (sym (apropos-internal regexp))

(This is from a `cl-defmethod' for `xref-backend-apropos'.)

Does this mean we have to preload apropos.el?  I don't understand how,
so I'm probably missing something.

Thanks for commenting.

Message #17 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 44529 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Tue, 10 Nov 2020 05:27:40 +0200

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Mon, 9 Nov 2020 12:44:26 -0800
> Cc: 44529 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> 
> > A bother: apropos.el is not preloaded, so please double-check that
> > none of the preloaded files call apropos-internal, otherwise that
> > would mean we need to preload apropos.el, which I think is
> > undesirable.
> 
> I only see one call in a preloaded file:
> 
> ./lisp/progmodes/elisp-mode.el:873: (dolist (sym (apropos-internal regexp))
> 
> (This is from a `cl-defmethod' for `xref-backend-apropos'.)
> 
> Does this mean we have to preload apropos.el?

Or move this Lisp implementation to another file, which is already
preloaded.

> I don't understand how, so I'm probably missing something.

Because if we don't, every Emacs session will load it right at the
start.

Message #20 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 44529 <at> debbugs.gnu.org, Stefan Kangas <stefan <at> marxist.se>
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Tue, 10 Nov 2020 08:14:01 -0500

> Because if we don't, every Emacs session will load it right at the
> start.

I don't see that: elisp-mode.el calls `apropos-internal` only from
`xref-backend-apropos`, which AFAICT is not used "right at the
start" but only after loading `xref.el` which is itself not preloaded.


        Stefan

Message #23 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 44529 <at> debbugs.gnu.org, stefan <at> marxist.se
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Tue, 10 Nov 2020 18:01:33 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Stefan Kangas <stefan <at> marxist.se>,  44529 <at> debbugs.gnu.org
> Date: Tue, 10 Nov 2020 08:14:01 -0500
> 
> > Because if we don't, every Emacs session will load it right at the
> > start.
> 
> I don't see that: elisp-mode.el calls `apropos-internal` only from
> `xref-backend-apropos`, which AFAICT is not used "right at the
> start" but only after loading `xref.el` which is itself not preloaded.

I don't want to rely on such shaky assumptions: the code in
elisp-mode.el can change any moment.

apropos-internal was always available in Emacs, so let's leave it
always available by putting it in subr.el or somesuch.

Message #26 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 44529 <at> debbugs.gnu.org, stefan <at> marxist.se
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Tue, 10 Nov 2020 14:50:02 -0500

>> I don't see that: elisp-mode.el calls `apropos-internal` only from
>> `xref-backend-apropos`, which AFAICT is not used "right at the
>> start" but only after loading `xref.el` which is itself not preloaded.
>
> I don't want to rely on such shaky assumptions: the code in
> elisp-mode.el can change any moment.
>
> apropos-internal was always available in Emacs, so let's leave it
> always available by putting it in subr.el or somesuch.

If we autoload it, it will work just as well without having to move it
outside of its most natural habitat.


        Stefan

Message #29 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 44529 <at> debbugs.gnu.org, stefan <at> marxist.se
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Tue, 10 Nov 2020 22:08:17 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: stefan <at> marxist.se,  44529 <at> debbugs.gnu.org
> Date: Tue, 10 Nov 2020 14:50:02 -0500
> 
> > apropos-internal was always available in Emacs, so let's leave it
> > always available by putting it in subr.el or somesuch.
> 
> If we autoload it, it will work just as well without having to move it
> outside of its most natural habitat.

I don't want it to become autoloaded, it could break something.  Why
risk that?

Message #32 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 44529 <at> debbugs.gnu.org
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Sun, 15 Nov 2020 17:07:14 -0800

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> If we autoload it, it will work just as well without having to move it
>> outside of its most natural habitat.
>
> I don't want it to become autoloaded, it could break something.  Why
> risk that?

The attached patch moves it to subr.el instead, as requested.

[0001-Convert-apropos-internal-from-C-to-Lisp.patch (text/x-diff, attachment)]

Message #35 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 44529 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Mon, 16 Nov 2020 19:52:46 +0200

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Sun, 15 Nov 2020 17:07:14 -0800
> Cc: 44529 <at> debbugs.gnu.org
> 
> The attached patch moves it to subr.el instead, as requested.

Thanks.

> This runs insignificantly faster in C, and is already fast enough on
> reasonably modern hardware.  We might as well lift it to Lisp.
> This benchmark can be used to verify:
> 
>   (benchmark-run 10 (apropos-command "test"))
>   => (0.12032415399999999 2 0.014772391999999995) ; C
>   => (0.13513192100000002 2 0.017216643000000004) ; Lisp

Btw, did you try with less trivial strings?  E.g., what happens if you
replace "test" with "set" or "file"?

Message #38 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 44529 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Tue, 24 Nov 2020 17:57:27 -0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Stefan Kangas <stefan <at> marxist.se>
>> Date: Sun, 15 Nov 2020 17:07:14 -0800
>> Cc: 44529 <at> debbugs.gnu.org
>>
>> The attached patch moves it to subr.el instead, as requested.
>
> Thanks.
>
>> This runs insignificantly faster in C, and is already fast enough on
>> reasonably modern hardware.  We might as well lift it to Lisp.
>> This benchmark can be used to verify:
>>
>>   (benchmark-run 10 (apropos-command "test"))
>>   => (0.12032415399999999 2 0.014772391999999995) ; C
>>   => (0.13513192100000002 2 0.017216643000000004) ; Lisp
>
> Btw, did you try with less trivial strings?  E.g., what happens if you
> replace "test" with "set" or "file"?

(Sorry for the late reply.)

I see results consistent with the previously reported 10 % difference,
or even slightly better than that:

(benchmark-run 10 (apropos-command "file"))
=> (0.6285004230000001 26 0.176056903)  ; C
=> (0.667433809 26 0.19296271500000006) ; Lisp

(/ 0.667433809 0.6285004230000001) => 1.0619

(benchmark-run 10 (apropos-command "set"))
=> (0.463329467 19 0.12921543499999996) ; C
=> (0.486383881 19 0.13347332099999998) ; Lisp

(/ 0.486383881 0.463329467) => 1.0497

Message #41 received at 44529 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 44529 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#44529: [PATCH] Convert apropos-internal from C to Lisp
Date: Sat, 19 Dec 2020 12:02:49 -0600

tags 44529 fixed
close 44529 28.1
thanks

Eli Zaretskii <eliz <at> gnu.org> writes:

>> The attached patch moves it to subr.el instead, as requested.
>
> Thanks.

No further comments within a month; pushed to master as commit
49ae715966.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.