bug#50777: Dropping EIEIO from xref (for performance)

bug#50777: Dropping EIEIO from xref (for performance)

[Dmitry Gutov]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

X-Debbugs-CC: Eli Zaretskii <eliz@gnu.org>, Lars Ingebrigtsen 
<larsi@gnus.org>, Stefan Monnier <monnier@iro.umontreal.ca>, Daniel 
Martín <mardani29@yahoo.es>, Mattias Engdegård <mattiase@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?

[-- Attachment #2: xref-from-defclass-to-defstruct.diff --]
[-- Type: text/x-patch, Size: 11380 bytes --]

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index e6af2b12c7..f53b09d9e8 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -2161,18 +2161,16 @@ etags--xref-apropos-additional
          (nreverse res))))
    tags-apropos-additional-actions))
 
-(defclass xref-etags-location (xref-location)
-  ((tag-info :type list   :initarg :tag-info)
-   (file     :type string :initarg :file
-             :reader xref-location-group))
-  :documentation "Location of an etags tag.")
+(cl-defstruct (xref-etags-location
+               (:constructor xref-make-etags-location (tag-info file)))
+  "Location of an etags tag."
+  tag-info file)
 
-(defun xref-make-etags-location (tag-info file)
-  (make-instance 'xref-etags-location :tag-info tag-info
-                 :file (expand-file-name file)))
+(cl-defmethod xref-location-group ((l xref-etags-location))
+  (xref-etags-location-file l))
 
 (cl-defmethod xref-location-marker ((l xref-etags-location))
-  (with-slots (tag-info file) l
+  (pcase-let (((cl-struct xref-etags-location tag-info file) l))
     (let ((buffer (find-file-noselect file)))
       (with-current-buffer buffer
         (save-excursion
@@ -2182,25 +2180,20 @@ xref-location-marker
             (point-marker)))))))
 
 (cl-defmethod xref-location-line ((l xref-etags-location))
-  (with-slots (tag-info) l
+  (pcase-let (((cl-struct xref-etags-location tag-info) l))
     (nth 1 tag-info)))
 
-(defclass xref-etags-apropos-location (xref-location)
-  ((symbol :type symbol :initarg :symbol)
-   (goto-fun :type function :initarg :goto-fun)
-   (group :type string :initarg :group
-          :reader xref-location-group))
-  :documentation "Location of an additional apropos etags symbol.")
+(cl-defstruct (xref-etags-apropos-location
+               (:constructor xref-make-etags-apropos-location (symbol goto-fun group)))
+  "Location of an additional apropos etags symbol."
+  symbol goto-fun group)
 
-(defun xref-make-etags-apropos-location (symbol goto-fun group)
-  (make-instance 'xref-etags-apropos-location
-                 :symbol symbol
-                 :goto-fun goto-fun
-                 :group group))
+(cl-defmethod xref-location-group ((l xref-etags-apropos-location))
+  (xref-etags-apropos-location-group l))
 
 (cl-defmethod xref-location-marker ((l xref-etags-apropos-location))
   (save-window-excursion
-    (with-slots (goto-fun symbol) l
+    (pcase-let (((cl-struct xref-etags-apropos-location goto-fun symbol) l))
       (funcall goto-fun symbol)
       (point-marker))))
 
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index fb8090cfb7..fa053bc987 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -78,9 +78,6 @@ xref
 \f
 ;;; Locations
 
-(defclass xref-location () ()
-  :documentation "A location represents a position in a file or buffer.")
-
 (cl-defgeneric xref-location-marker (location)
   "Return the marker for LOCATION.")
 
@@ -121,19 +118,20 @@ xref-file-name-display
 
 ;; FIXME: might be useful to have an optional "hint" i.e. a string to
 ;; search for in case the line number is slightly out of date.
-(defclass xref-file-location (xref-location)
-  ((file :type string :initarg :file :reader xref-location-group)
-   (line :type fixnum :initarg :line :reader xref-location-line)
-   (column :type fixnum :initarg :column :reader xref-file-location-column))
-  :documentation "A file location is a file/line/column triple.
-Line numbers start from 1 and columns from 0.")
+(cl-defstruct (xref-file-location
+               (:constructor xref-make-file-location (file line column)))
+  "A file location is a file/line/column triple.
+Line numbers start from 1 and columns from 0."
+  file line column)
+
+(cl-defmethod xref-location-group ((l xref-file-location))
+  (xref-file-location-file l))
 
-(defun xref-make-file-location (file line column)
-  "Create and return a new `xref-file-location'."
-  (make-instance 'xref-file-location :file file :line line :column column))
+(cl-defmethod xref-location-line ((l xref-file-location))
+  (xref-file-location-line l))
 
 (cl-defmethod xref-location-marker ((l xref-file-location))
-  (with-slots (file line column) l
+  (pcase-let (((cl-struct xref-file-location file line column) l))
     (with-current-buffer
         (or (get-file-buffer file)
             (let ((find-file-suppress-same-file-warnings t))
@@ -151,77 +149,45 @@ xref-location-marker
             (forward-char column))
           (point-marker))))))
 
-(defclass xref-buffer-location (xref-location)
-  ((buffer :type buffer :initarg :buffer)
-   (position :type fixnum :initarg :position)))
-
-(defun xref-make-buffer-location (buffer position)
-  "Create and return a new `xref-buffer-location'."
-  (make-instance 'xref-buffer-location :buffer buffer :position position))
+(cl-defstruct (xref-buffer-location
+               (:constructor xref-make-buffer-location (buffer position)))
+  buffer position)
 
 (cl-defmethod xref-location-marker ((l xref-buffer-location))
-  (with-slots (buffer position) l
+  (pcase-let (((cl-struct xref-buffer-location buffer position) l))
     (let ((m (make-marker)))
       (move-marker m position buffer))))
 
 (cl-defmethod xref-location-group ((l xref-buffer-location))
-  (with-slots (buffer) l
+  (pcase-let (((cl-struct xref-buffer-location buffer) l))
     (or (buffer-file-name buffer)
         (format "(buffer %s)" (buffer-name buffer)))))
 
-(defclass xref-bogus-location (xref-location)
-  ((message :type string :initarg :message
-            :reader xref-bogus-location-message))
-  :documentation "Bogus locations are sometimes useful to
-indicate errors, e.g. when we know that a function exists but the
-actual location is not known.")
-
-(defun xref-make-bogus-location (message)
-  "Create and return a new `xref-bogus-location'."
-  (make-instance 'xref-bogus-location :message message))
+(cl-defstruct (xref-bogus-location
+               (:constructor xref-make-bogus-location (message)))
+  "Bogus locations are sometimes useful to indicate errors,
+e.g. when we know that a function exists but the actual location
+is not known."
+  message)
 
 (cl-defmethod xref-location-marker ((l xref-bogus-location))
-  (user-error "%s" (oref l message)))
+  (user-error "%s" (xref-bogus-location-message l)))
 
 (cl-defmethod xref-location-group ((_ xref-bogus-location)) "(No location)")
 
 \f
 ;;; Cross-reference
 
-(defclass xref-item ()
-  ((summary :type string :initarg :summary
-            :reader xref-item-summary
-            :documentation "One line which will be displayed for
-this item in the output buffer.")
-   (location :initarg :location
-             :reader xref-item-location
-             :documentation "An object describing how to navigate
-to the reference's target."))
-  :comment "An xref item describes a reference to a location
-somewhere.")
-
-(defun xref-make (summary location)
-  "Create and return a new `xref-item'.
-SUMMARY is a short string to describe the xref.
-LOCATION is an `xref-location'."
-  (make-instance 'xref-item :summary summary :location location))
-
-(defclass xref-match-item ()
-  ((summary :type string :initarg :summary
-            :reader xref-item-summary)
-   (location :initarg :location
-             :type xref-location
-             :reader xref-item-location)
-   (length :initarg :length :reader xref-match-length))
-  :comment "A match xref item describes a search result.")
-
-(defun xref-make-match (summary location length)
-  "Create and return a new `xref-match-item'.
-SUMMARY is a short string to describe the xref.
-LOCATION is an `xref-location'.
-LENGTH is the match length, in characters."
-  (make-instance 'xref-match-item :summary summary
-                 :location location :length length))
+(cl-defstruct (xref-item
+               (:constructor xref-make (summary location)))
+  "An xref item describes a reference to a location somewhere."
+  summary location)
+
+(cl-defstruct (xref-match-item
+               (:include xref-item)
+               (:constructor xref-make-match (summary location length)))
+  "A match xref item describes a search result."
+  length)
 
 \f
 ;;; API
@@ -970,7 +936,7 @@ xref--insert-xrefs
            for max-line-width =
            (cl-loop for xref in xrefs
                     maximize (let ((line (xref-location-line
-                                          (oref xref location))))
+                                          (xref-item-location xref))))
                                (and line (1+ (floor (log line 10))))))
            for line-format = (and max-line-width
                                   (format "%%%dd: " max-line-width))
@@ -985,7 +951,7 @@ xref--insert-xrefs
            (xref--insert-propertized '(face xref-file-header xref-group t)
                                      group "\n")
            (cl-loop for xref in xrefs do
-                    (with-slots (summary location) xref
+                    (pcase-let (((cl-struct xref-item summary location) xref))
                       (let* ((line (xref-location-line location))
                              (prefix
                               (cond
@@ -1206,22 +1172,23 @@ xref-show-definitions-completing-read
     (cl-loop for ((group . xrefs) . more1) on xref-alist
              do
              (cl-loop for (xref . more2) on xrefs do
-                      (with-slots (summary location) xref
-                        (let* ((line (xref-location-line location))
-                               (line-fmt
-                                (if line
-                                    (format #("%d:" 0 2 (face xref-line-number))
-                                            line)
-                                  ""))
-                               (group-prefix
-                                (substring group group-prefix-length))
-                               (group-fmt
-                                (propertize group-prefix
-                                            'face 'xref-file-header
-                                            'xref--group group-prefix))
-                               (candidate
-                                (format "%s:%s%s" group-fmt line-fmt summary)))
-                          (push (cons candidate xref) xref-alist-with-line-info)))))
+                      (let* ((summary (xref-item-summary xref))
+                             (location (xref-item-location xref))
+                             (line (xref-location-line location))
+                             (line-fmt
+                              (if line
+                                  (format #("%d:" 0 2 (face xref-line-number))
+                                          line)
+                                ""))
+                             (group-prefix
+                              (substring group group-prefix-length))
+                             (group-fmt
+                              (propertize group-prefix
+                                          'face 'xref-file-header
+                                          'xref--group group-prefix))
+                             (candidate
+                              (format "%s:%s%s" group-fmt line-fmt summary)))
+                        (push (cons candidate xref) xref-alist-with-line-info))))
 
     (setq xref (if (not (cdr xrefs))
                    (car xrefs)

bug#50777: Dropping EIEIO from xref (for performance)

[Dmitry Gutov]

On 24.09.2021 16:28, Dmitry Gutov wrote:
> X-Debbugs-CC: Eli Zaretskii <eliz@gnu.org>, Lars Ingebrigtsen 
> <larsi@gnus.org>, Stefan Monnier <monnier@iro.umontreal.ca>, Daniel 
> Martín <mardani29@yahoo.es>, Mattias Engdegård <mattiase@acm.org>

Anybody who wants to argue Debbugs is really usable, check out the Cc it 
generated from the above string.

bug#50777: Dropping EIEIO from xref (for performance)

[Dmitry Gutov]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

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?

[-- Attachment #2: xref-from-defclass-to-defstruct.diff --]
[-- Type: text/x-patch, Size: 11380 bytes --]

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index e6af2b12c7..f53b09d9e8 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -2161,18 +2161,16 @@ etags--xref-apropos-additional
          (nreverse res))))
    tags-apropos-additional-actions))
 
-(defclass xref-etags-location (xref-location)
-  ((tag-info :type list   :initarg :tag-info)
-   (file     :type string :initarg :file
-             :reader xref-location-group))
-  :documentation "Location of an etags tag.")
+(cl-defstruct (xref-etags-location
+               (:constructor xref-make-etags-location (tag-info file)))
+  "Location of an etags tag."
+  tag-info file)
 
-(defun xref-make-etags-location (tag-info file)
-  (make-instance 'xref-etags-location :tag-info tag-info
-                 :file (expand-file-name file)))
+(cl-defmethod xref-location-group ((l xref-etags-location))
+  (xref-etags-location-file l))
 
 (cl-defmethod xref-location-marker ((l xref-etags-location))
-  (with-slots (tag-info file) l
+  (pcase-let (((cl-struct xref-etags-location tag-info file) l))
     (let ((buffer (find-file-noselect file)))
       (with-current-buffer buffer
         (save-excursion
@@ -2182,25 +2180,20 @@ xref-location-marker
             (point-marker)))))))
 
 (cl-defmethod xref-location-line ((l xref-etags-location))
-  (with-slots (tag-info) l
+  (pcase-let (((cl-struct xref-etags-location tag-info) l))
     (nth 1 tag-info)))
 
-(defclass xref-etags-apropos-location (xref-location)
-  ((symbol :type symbol :initarg :symbol)
-   (goto-fun :type function :initarg :goto-fun)
-   (group :type string :initarg :group
-          :reader xref-location-group))
-  :documentation "Location of an additional apropos etags symbol.")
+(cl-defstruct (xref-etags-apropos-location
+               (:constructor xref-make-etags-apropos-location (symbol goto-fun group)))
+  "Location of an additional apropos etags symbol."
+  symbol goto-fun group)
 
-(defun xref-make-etags-apropos-location (symbol goto-fun group)
-  (make-instance 'xref-etags-apropos-location
-                 :symbol symbol
-                 :goto-fun goto-fun
-                 :group group))
+(cl-defmethod xref-location-group ((l xref-etags-apropos-location))
+  (xref-etags-apropos-location-group l))
 
 (cl-defmethod xref-location-marker ((l xref-etags-apropos-location))
   (save-window-excursion
-    (with-slots (goto-fun symbol) l
+    (pcase-let (((cl-struct xref-etags-apropos-location goto-fun symbol) l))
       (funcall goto-fun symbol)
       (point-marker))))
 
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index fb8090cfb7..fa053bc987 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -78,9 +78,6 @@ xref
 \f
 ;;; Locations
 
-(defclass xref-location () ()
-  :documentation "A location represents a position in a file or buffer.")
-
 (cl-defgeneric xref-location-marker (location)
   "Return the marker for LOCATION.")
 
@@ -121,19 +118,20 @@ xref-file-name-display
 
 ;; FIXME: might be useful to have an optional "hint" i.e. a string to
 ;; search for in case the line number is slightly out of date.
-(defclass xref-file-location (xref-location)
-  ((file :type string :initarg :file :reader xref-location-group)
-   (line :type fixnum :initarg :line :reader xref-location-line)
-   (column :type fixnum :initarg :column :reader xref-file-location-column))
-  :documentation "A file location is a file/line/column triple.
-Line numbers start from 1 and columns from 0.")
+(cl-defstruct (xref-file-location
+               (:constructor xref-make-file-location (file line column)))
+  "A file location is a file/line/column triple.
+Line numbers start from 1 and columns from 0."
+  file line column)
+
+(cl-defmethod xref-location-group ((l xref-file-location))
+  (xref-file-location-file l))
 
-(defun xref-make-file-location (file line column)
-  "Create and return a new `xref-file-location'."
-  (make-instance 'xref-file-location :file file :line line :column column))
+(cl-defmethod xref-location-line ((l xref-file-location))
+  (xref-file-location-line l))
 
 (cl-defmethod xref-location-marker ((l xref-file-location))
-  (with-slots (file line column) l
+  (pcase-let (((cl-struct xref-file-location file line column) l))
     (with-current-buffer
         (or (get-file-buffer file)
             (let ((find-file-suppress-same-file-warnings t))
@@ -151,77 +149,45 @@ xref-location-marker
             (forward-char column))
           (point-marker))))))
 
-(defclass xref-buffer-location (xref-location)
-  ((buffer :type buffer :initarg :buffer)
-   (position :type fixnum :initarg :position)))
-
-(defun xref-make-buffer-location (buffer position)
-  "Create and return a new `xref-buffer-location'."
-  (make-instance 'xref-buffer-location :buffer buffer :position position))
+(cl-defstruct (xref-buffer-location
+               (:constructor xref-make-buffer-location (buffer position)))
+  buffer position)
 
 (cl-defmethod xref-location-marker ((l xref-buffer-location))
-  (with-slots (buffer position) l
+  (pcase-let (((cl-struct xref-buffer-location buffer position) l))
     (let ((m (make-marker)))
       (move-marker m position buffer))))
 
 (cl-defmethod xref-location-group ((l xref-buffer-location))
-  (with-slots (buffer) l
+  (pcase-let (((cl-struct xref-buffer-location buffer) l))
     (or (buffer-file-name buffer)
         (format "(buffer %s)" (buffer-name buffer)))))
 
-(defclass xref-bogus-location (xref-location)
-  ((message :type string :initarg :message
-            :reader xref-bogus-location-message))
-  :documentation "Bogus locations are sometimes useful to
-indicate errors, e.g. when we know that a function exists but the
-actual location is not known.")
-
-(defun xref-make-bogus-location (message)
-  "Create and return a new `xref-bogus-location'."
-  (make-instance 'xref-bogus-location :message message))
+(cl-defstruct (xref-bogus-location
+               (:constructor xref-make-bogus-location (message)))
+  "Bogus locations are sometimes useful to indicate errors,
+e.g. when we know that a function exists but the actual location
+is not known."
+  message)
 
 (cl-defmethod xref-location-marker ((l xref-bogus-location))
-  (user-error "%s" (oref l message)))
+  (user-error "%s" (xref-bogus-location-message l)))
 
 (cl-defmethod xref-location-group ((_ xref-bogus-location)) "(No location)")
 
 \f
 ;;; Cross-reference
 
-(defclass xref-item ()
-  ((summary :type string :initarg :summary
-            :reader xref-item-summary
-            :documentation "One line which will be displayed for
-this item in the output buffer.")
-   (location :initarg :location
-             :reader xref-item-location
-             :documentation "An object describing how to navigate
-to the reference's target."))
-  :comment "An xref item describes a reference to a location
-somewhere.")
-
-(defun xref-make (summary location)
-  "Create and return a new `xref-item'.
-SUMMARY is a short string to describe the xref.
-LOCATION is an `xref-location'."
-  (make-instance 'xref-item :summary summary :location location))
-
-(defclass xref-match-item ()
-  ((summary :type string :initarg :summary
-            :reader xref-item-summary)
-   (location :initarg :location
-             :type xref-location
-             :reader xref-item-location)
-   (length :initarg :length :reader xref-match-length))
-  :comment "A match xref item describes a search result.")
-
-(defun xref-make-match (summary location length)
-  "Create and return a new `xref-match-item'.
-SUMMARY is a short string to describe the xref.
-LOCATION is an `xref-location'.
-LENGTH is the match length, in characters."
-  (make-instance 'xref-match-item :summary summary
-                 :location location :length length))
+(cl-defstruct (xref-item
+               (:constructor xref-make (summary location)))
+  "An xref item describes a reference to a location somewhere."
+  summary location)
+
+(cl-defstruct (xref-match-item
+               (:include xref-item)
+               (:constructor xref-make-match (summary location length)))
+  "A match xref item describes a search result."
+  length)
 
 \f
 ;;; API
@@ -970,7 +936,7 @@ xref--insert-xrefs
            for max-line-width =
            (cl-loop for xref in xrefs
                     maximize (let ((line (xref-location-line
-                                          (oref xref location))))
+                                          (xref-item-location xref))))
                                (and line (1+ (floor (log line 10))))))
            for line-format = (and max-line-width
                                   (format "%%%dd: " max-line-width))
@@ -985,7 +951,7 @@ xref--insert-xrefs
            (xref--insert-propertized '(face xref-file-header xref-group t)
                                      group "\n")
            (cl-loop for xref in xrefs do
-                    (with-slots (summary location) xref
+                    (pcase-let (((cl-struct xref-item summary location) xref))
                       (let* ((line (xref-location-line location))
                              (prefix
                               (cond
@@ -1206,22 +1172,23 @@ xref-show-definitions-completing-read
     (cl-loop for ((group . xrefs) . more1) on xref-alist
              do
              (cl-loop for (xref . more2) on xrefs do
-                      (with-slots (summary location) xref
-                        (let* ((line (xref-location-line location))
-                               (line-fmt
-                                (if line
-                                    (format #("%d:" 0 2 (face xref-line-number))
-                                            line)
-                                  ""))
-                               (group-prefix
-                                (substring group group-prefix-length))
-                               (group-fmt
-                                (propertize group-prefix
-                                            'face 'xref-file-header
-                                            'xref--group group-prefix))
-                               (candidate
-                                (format "%s:%s%s" group-fmt line-fmt summary)))
-                          (push (cons candidate xref) xref-alist-with-line-info)))))
+                      (let* ((summary (xref-item-summary xref))
+                             (location (xref-item-location xref))
+                             (line (xref-location-line location))
+                             (line-fmt
+                              (if line
+                                  (format #("%d:" 0 2 (face xref-line-number))
+                                          line)
+                                ""))
+                             (group-prefix
+                              (substring group group-prefix-length))
+                             (group-fmt
+                              (propertize group-prefix
+                                          'face 'xref-file-header
+                                          'xref--group group-prefix))
+                             (candidate
+                              (format "%s:%s%s" group-fmt line-fmt summary)))
+                        (push (cons candidate xref) xref-alist-with-line-info))))
 
     (setq xref (if (not (cdr xrefs))
                    (car xrefs)

bug#50777: Dropping EIEIO from xref (for performance)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

bug#50777: Dropping EIEIO from xref (for performance)

[Arthur Miller]

Dmitry Gutov <dgutov@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.

bug#50777: Dropping EIEIO from xref (for performance)

[Dmitry Gutov]

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.

bug#50777: Dropping EIEIO from xref (for performance)

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@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

bug#50777: Dropping EIEIO from xref (for performance)

[Dmitry Gutov]

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.

bug#50777: Dropping EIEIO from xref (for performance)

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dgutov@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.

bug#50777: Dropping EIEIO from xref (for performance)

[Dmitry Gutov]

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.

bug#50777: Dropping EIEIO from xref (for performance)

[Dmitry Gutov]

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.

bug#50777: Dropping EIEIO from xref (for performance)

[Mattias Engdegård]

26 sep. 2021 kl. 03.15 skrev Dmitry Gutov <dgutov@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.

bug#50777: Dropping EIEIO from xref (for performance)

[Dmitry Gutov]

On 26.09.2021 11:51, Mattias Engdegård wrote:
> 26 sep. 2021 kl. 03.15 skrev Dmitry Gutov <dgutov@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.

bug#50777: Dropping EIEIO from xref (for performance)

[Dmitry Gutov]

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.