GNU bug report logs -
#50777
Dropping EIEIO from xref (for performance)
Previous Next
Reported by: Dmitry Gutov <dgutov <at> yandex.ru>
Date: Fri, 24 Sep 2021 13:30:02 UTC
Severity: normal
Done: Dmitry Gutov <dgutov <at> yandex.ru>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 50777 in the body.
You can then email your comments to 50777 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
eliz <at> gnu.org, lars, ingebrigtsen, bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Fri, 24 Sep 2021 13:30:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Dmitry Gutov <dgutov <at> yandex.ru>:
New bug report received and forwarded. Copy sent to
eliz <at> gnu.org, lars, ingebrigtsen, bug-gnu-emacs <at> gnu.org.
(Fri, 24 Sep 2021 13:30:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
X-Debbugs-CC: Eli Zaretskii <eliz <at> gnu.org>, Lars Ingebrigtsen
<larsi <at> gnus.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>, Daniel
Martín <mardani29 <at> yahoo.es>, Mattias Engdegård <mattiase <at> acm.org>
With the recent discussion about our Lisp overhead when the search
returns many matches, I went a couple more rounds to try to identify the
hotspots.
My main benchmark for that is
(benchmark 1 '(project-find-regexp "list"))
in the Emacs repo. The 3 recent commits have reduced that time from 8s
to 6.5s on my machine (under best conditions: warm disk cache and fresh
Emacs session). The machine is pretty fast, so I figure we can multiply
that timing by 2x, to imagine the average user experience.
Having exhausted the obvious steps, I have looked at the difference
between defclass and cl-defstruct in this respect. Earlier quick tests
indicated there would be little to no difference, but now that I've
wrote the full patch, it's significant.
The attached patch drops the timing of the above benchmark further down
to 4.1s here.
Both allocation ('make-instance' vs auto-generated 'xref-make-*' struct
constructors) and accessing the fields show increased performance, with
the former showing the most improvement: I guess EIEIO's type checking
added some extra overhead.
That creates some incompatibility (third-party packages can't inherit
from 'xref-location' anymore, or use 'make-instance', 'oref' or
'with-slots' with our values), but OTOH a quick survey of existing
packages that integrate with Xref shows they wisely don't do that
already. Not the largest, popular ones I have reviewed, at least.
The ones that do can be updated to rely on the recommended public
interface: the xref-make-* constructors and the generic functions to
access the data. This will keep the code compatible with both new and
previous versions of Xref.
I'd like to push it soon, so users of Emacs 28 can enjoy the speedup.
What do people think?
[xref-from-defclass-to-defstruct.diff (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Fri, 24 Sep 2021 13:36:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 50777 <at> debbugs.gnu.org (full text, mbox):
On 24.09.2021 16:28, Dmitry Gutov wrote:
> X-Debbugs-CC: Eli Zaretskii <eliz <at> gnu.org>, Lars Ingebrigtsen
> <larsi <at> gnus.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>, Daniel
> Martín <mardani29 <at> yahoo.es>, Mattias Engdegård <mattiase <at> acm.org>
Anybody who wants to argue Debbugs is really usable, check out the Cc it
generated from the above string.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Fri, 24 Sep 2021 13:38:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 50777 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Resending with the intended recipients (except for the maintainers who
are automatically subscribed anyway, to spare the inboxes).
With the recent discussion about our Lisp overhead when the search
returns many matches, I went a couple more rounds to try to identify the
hotspots.
My main benchmark for that is
(benchmark 1 '(project-find-regexp "list"))
in the Emacs repo. The 3 recent commits have reduced that time from 8s
to 6.5s on my machine (under best conditions: warm disk cache and fresh
Emacs session). The machine is pretty fast, so I figure we can multiply
that timing by 2x, to imagine the average user experience.
Having exhausted the obvious steps, I have looked at the difference
between defclass and cl-defstruct in this respect. Earlier quick tests
indicated there would be little to no difference, but now that I've
wrote the full patch, it's significant.
The attached patch drops the timing of the above benchmark further down
to 4.1s here.
Both allocation ('make-instance' vs auto-generated 'xref-make-*' struct
constructors) and accessing the fields show increased performance, with
the former showing the most improvement: I guess EIEIO's type checking
added some extra overhead.
That creates some incompatibility (third-party packages can't inherit
from 'xref-location' anymore, or use 'make-instance', 'oref' or
'with-slots' with our values), but OTOH a quick survey of existing
packages that integrate with Xref shows they wisely don't do that
already. Not the largest, popular ones I have reviewed, at least.
The ones that do can be updated to rely on the recommended public
interface: the xref-make-* constructors and the generic functions to
access the data. This will keep the code compatible with both new and
previous versions of Xref.
I'd like to push it soon, so users of Emacs 28 can enjoy the speedup.
What do people think?
[xref-from-defclass-to-defstruct.diff (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Fri, 24 Sep 2021 14:50:01 GMT)
Full text and
rfc822 format available.
Message #14 received at 50777 <at> debbugs.gnu.org (full text, mbox):
> That creates some incompatibility (third-party packages can't inherit from
> 'xref-location' anymore, or use 'make-instance', 'oref' or 'with-slots' with
> our values),
About that: it should be fairly easy to get `make-instance`, `oref`, and
`with-slots` to work with cl-defstruct objects/classes.
This should be easy enough that we *could* potentially consider doing
those changes "right away" (i.e. for Emacs-28).
As for the inheritance, they can still inherit from `xref-location` but:
- not with `defclass` (the subclass needs to be defined with `cl-defstruct`
instead).
- they can't inherit from both `xref-location` and something else any more.
The second restriction could potentially be lifted without too much work
(we could allow multiple inheritance as long as only one of the parents
has fields, a bit like Java allows only one parent but multiple
interfaces where interfaces can be though of "classes without fields").
I think the first restriction can be lifted as well under the constraint
that `defclass` can only inherit from a single defstruct class that
should be the first parent (since its fields need to be placed at the
very beginning of the object).
But I don't think those two additional changes would be in scope of Emacs-28.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Fri, 24 Sep 2021 15:13:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 50777 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov <dgutov <at> yandex.ru> writes:
> Resending with the intended recipients (except for the maintainers who are
> automatically subscribed anyway, to spare the inboxes).
>
> With the recent discussion about our Lisp overhead when the search returns many
> matches, I went a couple more rounds to try to identify the hotspots.
>
> My main benchmark for that is
>
> (benchmark 1 '(project-find-regexp "list"))
>
> in the Emacs repo. The 3 recent commits have reduced that time from 8s to 6.5s
> on my machine (under best conditions: warm disk cache and fresh Emacs
> session). The machine is pretty fast, so I figure we can multiply that timing by
> 2x, to imagine the average user experience.
>
> Having exhausted the obvious steps, I have looked at the difference between
> defclass and cl-defstruct in this respect. Earlier quick tests indicated there
> would be little to no difference, but now that I've wrote the full patch, it's
> significant.
>
> The attached patch drops the timing of the above benchmark further down to 4.1s
> here.
>
> Both allocation ('make-instance' vs auto-generated 'xref-make-*' struct
> constructors) and accessing the fields show increased performance, with the
> former showing the most improvement: I guess EIEIO's type checking added some
> extra overhead.
>
> That creates some incompatibility (third-party packages can't inherit from
> 'xref-location' anymore, or use 'make-instance', 'oref' or 'with-slots' with our
> values), but OTOH a quick survey of existing packages that integrate with Xref
> shows they wisely don't do that already. Not the largest, popular ones I have
> reviewed, at least.
>
> The ones that do can be updated to rely on the recommended public interface: the
> xref-make-* constructors and the generic functions to access the data. This will
> keep the code compatible with both new and previous versions of Xref.
>
> I'd like to push it soon, so users of Emacs 28 can enjoy the speedup.
>
> What do people think?
Yes please.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Fri, 24 Sep 2021 15:33:01 GMT)
Full text and
rfc822 format available.
Message #20 received at 50777 <at> debbugs.gnu.org (full text, mbox):
On 24.09.2021 17:49, Stefan Monnier wrote:
>> That creates some incompatibility (third-party packages can't inherit from
>> 'xref-location' anymore, or use 'make-instance', 'oref' or 'with-slots' with
>> our values),
>
> About that: it should be fairly easy to get `make-instance`, `oref`, and
> `with-slots` to work with cl-defstruct objects/classes.
>
> This should be easy enough that we *could* potentially consider doing
> those changes "right away" (i.e. for Emacs-28).
Interesting point.
Not sure we really want to: I imagine the removal of the use of
'make-instance' contributed to the overall performance improvement. If
I'm right, discouraging of its use (for this particular purpose) in
third-party code seems like a good idea.
> As for the inheritance, they can still inherit from `xref-location` but:
> - not with `defclass` (the subclass needs to be defined with `cl-defstruct`
> instead).
> - they can't inherit from both `xref-location` and something else any more.
There is no 'xref-location' structure, it's only defined by generic
functions. I think that's better.
'xref-location' has always been a class with empty definition.
One area of bigger concern is whether code compiled against the new
version of Xref would work with the old one without recompiling (and
vice versa). I guess for that to work we need to disable inlining on
xref-item's accessors, at least. Maybe there's something else I'm missing.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Sat, 25 Sep 2021 01:44:01 GMT)
Full text and
rfc822 format available.
Message #23 received at 50777 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov <dgutov <at> yandex.ru> writes:
> The ones that do can be updated to rely on the recommended public
> interface: the xref-make-* constructors and the generic functions to
> access the data. This will keep the code compatible with both new and
> previous versions of Xref.
>
> I'd like to push it soon, so users of Emacs 28 can enjoy the speedup.
>
> What do people think?
Speedups would be nice, but I think this sounds like a very big change
to make to such a central system (that I think many third-party packages
rely on) at this point, so I'm leaning towards it being in Emacs 29
instead.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Sat, 25 Sep 2021 01:54:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 50777 <at> debbugs.gnu.org (full text, mbox):
On 25.09.2021 04:42, Lars Ingebrigtsen wrote:
> Speedups would be nice, but I think this sounds like a very big change
> to make to such a central system (that I think many third-party packages
> rely on) at this point, so I'm leaning towards it being in Emacs 29
> instead.
Xref is an "ELPA core" package. As soon as we make this change on master
and bump the version, it will affect the third-party packages right
away. Not when Emacs 29 is released, year(s) later.
On the flip side, we can make the change now, bump the version, and be
reasonably certain that when Emacs 28 comes out, the third-party
packages are already updated for the version that comes with it.
It's also the most reliable way to work out the kinks that I know of.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Sat, 25 Sep 2021 14:08:01 GMT)
Full text and
rfc822 format available.
Message #29 received at 50777 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov <dgutov <at> yandex.ru> writes:
> Resending with the intended recipients (except for the maintainers who
> are automatically subscribed anyway, to spare the inboxes).
>
> With the recent discussion about our Lisp overhead when the search
> returns many matches, I went a couple more rounds to try to identify
> the hotspots.
>
> My main benchmark for that is
>
> (benchmark 1 '(project-find-regexp "list"))
>
> in the Emacs repo. The 3 recent commits have reduced that time from 8s
> to 6.5s on my machine (under best conditions: warm disk cache and
> fresh Emacs session). The machine is pretty fast, so I figure we can
> multiply that timing by 2x, to imagine the average user experience.
Thanks for working on this. I checked the patch and I also see a
similar speedup in my system, working with a large monorepo.
>
> I'd like to push it soon, so users of Emacs 28 can enjoy the speedup.
>
> What do people think?
I agree with Lars, it's better not to rush this change. The change is
invasive and I think I was the only person to report a performance
problem so far in that part of the codebase. I guess it's better to
merge this just after the Emacs 28 branch is cut, so that users in a
stable version are not affected by any potential bug/interface
incompatibility not caught during pretest. Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Sun, 26 Sep 2021 01:16:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 50777 <at> debbugs.gnu.org (full text, mbox):
On 24.09.2021 18:32, Dmitry Gutov wrote:
> One area of bigger concern is whether code compiled against the new
> version of Xref would work with the old one without recompiling (and
> vice versa). I guess for that to work we need to disable inlining on
> xref-item's accessors, at least. Maybe there's something else I'm missing.
OK, so I've done some testing with ivy-xref (fixing it to stop using
with-slots), and (:noinline t) indeed seems necessary for Emacs using
previous version of xref to run code from ivy-xref.elc compiled with the
newer version without errors.
But since I tested this in Emacs 27,
(void-function make-closure)
was a much bigger problem. This is apparently how lambdas are compiled now.
So this is a +1 minor reason to release the new version together with
Emacs 28: less need to worry about :noinline. Though we could use it
anyway, to avoid tying byte code to a particular implementation: the
performance seems unchanged.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Sun, 26 Sep 2021 02:17:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 50777 <at> debbugs.gnu.org (full text, mbox):
On 25.09.2021 17:07, Daniel Martín wrote:
> Thanks for working on this. I checked the patch and I also see a
> similar speedup in my system, working with a large monorepo.
Thanks for checking.
Could you also do some benchmarking with GNU Grep, as requested in
bug#50733?
> I agree with Lars, it's better not to rush this change. The change is
> invasive and I think I was the only person to report a performance
> problem so far in that part of the codebase. I guess it's better to
> merge this just after the Emacs 28 branch is cut, so that users in a
> stable version are not affected by any potential bug/interface
> incompatibility not caught during pretest. Thanks.
It has been a known issue for a while, with repeated passes at improving
its performance (the "many matches" scenario).
Yours has been the second report I've received in the last couple of weeks.
I suppose it makes sense to be careful, but the only incompatible
packages I have found so far are helm-xref and ivy-xref, both only using
with-slots (copied verbatim from xref--insert-xrefs, I guess). It's easy
to fix, and the latter can mostly be replaced with the use of
xref-show-definitions-completing-read these days. Maybe both of them can.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Sun, 26 Sep 2021 08:52:01 GMT)
Full text and
rfc822 format available.
Message #38 received at 50777 <at> debbugs.gnu.org (full text, mbox):
26 sep. 2021 kl. 03.15 skrev Dmitry Gutov <dgutov <at> yandex.ru>:
> (void-function make-closure)
>
> was a much bigger problem. This is apparently how lambdas are compiled now.
There are no circumstances under which Emacs 28 bytecode is guaranteed to work in earlier Emacs versions: there is backward but no forward compatibility. For example, any code using `with-temp-buffer` won't work, nor as you noticed anything creating closures.
This is unavoidable and the alternative would be much worse, but it's sometimes annoying when testing code with different versions and (in particular) installing packages using a new version and then using an old one.
Could we arrange the .el file to be used if the .elc is built with a newer Emacs version? (Perversely, it would make some inlined functions faster...)
An alternative would be to segregate installed packages by (major) version, so that ~/.emacs.d/ has separate installations for Emacs 26, 27, 28 and so on.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50777; Package
emacs.
(Sun, 26 Sep 2021 18:35:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 50777 <at> debbugs.gnu.org (full text, mbox):
On 26.09.2021 11:51, Mattias Engdegård wrote:
> 26 sep. 2021 kl. 03.15 skrev Dmitry Gutov <dgutov <at> yandex.ru>:
>
>> (void-function make-closure)
>>
>> was a much bigger problem. This is apparently how lambdas are compiled now.
>
> There are no circumstances under which Emacs 28 bytecode is guaranteed to work in earlier Emacs versions: there is backward but no forward compatibility. For example, any code using `with-temp-buffer` won't work, nor as you noticed anything creating closures.
It's all right. Sometimes the compiled code is compatible, but sometimes
it's not. We could introduce format versions, maybe. Though it's hard to
draw the line because it's probably not the byte-code format itself that
changed, but simply certain representations of primitives.
I was just saying that pushing that change now might allow us to stop
worrying about a particular incompatibility. But we'll probably disable
inlining for xref-item accessors anyway, to allow a similar migration to
another underlying data format in the future. Just in case.
> This is unavoidable and the alternative would be much worse, but it's sometimes annoying when testing code with different versions and (in particular) installing packages using a new version and then using an old one.
True.
> Could we arrange the .el file to be used if the .elc is built with a newer Emacs version? (Perversely, it would make some inlined functions faster...)
Sometimes it will be faster, but sometimes much slower (2x or more). I'm
not sure what's the best solution here. Maybe loading .el files *and*
nagging the user once per session to recompile their packages could work.
> An alternative would be to segregate installed packages by (major) version, so that ~/.emacs.d/ has separate installations for Emacs 26, 27, 28 and so on.
Sounds clunky, but maybe it's the right choice. Or maybe move only the
compiled files to a separate directory (or directories), and version
them by byte-code format. Or major Emacs version.
Reply sent
to
Dmitry Gutov <dgutov <at> yandex.ru>:
You have taken responsibility.
(Thu, 30 Sep 2021 21:11:01 GMT)
Full text and
rfc822 format available.
Notification sent
to
Dmitry Gutov <dgutov <at> yandex.ru>:
bug acknowledged by developer.
(Thu, 30 Sep 2021 21:11:02 GMT)
Full text and
rfc822 format available.
Message #46 received at 50777-done <at> debbugs.gnu.org (full text, mbox):
In the absence of strong objections, pushed to Emacs 28.
Sorry about jumping into a departing train: I had a flight this morning
and had very little time the night before.
In addition to other considerations, removing the unjustified
restriction of ':type xref-location' on xref-match-item's location
should also have a good effect on the ecosystem.
Thanks all.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Fri, 29 Oct 2021 11:24:07 GMT)
Full text and
rfc822 format available.
This bug report was last modified 2 years and 172 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.