search

seagle0128 / doom-modeline

A fancy and fast mode-line inspired by minimalism design.
https://seagle0128.github.io/doom-modeline/
GNU General Public License v3.0
1.28k stars 158 forks source link

"function provided already compiled" and Emacs freezes #601

Closed maxecharel closed 1 year ago

maxecharel commented 1 year ago

Thank you for the bug report

Bug description

I guess just after an update of doom-modeline (I batch-updated packages 10 minutes ago), when I restarted Emacs I got the message "function provided already compiled" and Emacs froze. I had to kill the process (no way to even enter a command). Commenting out doom-modeline in my init file solved the issue, so I conclude the problem comes from it. (Thx for your work with this really classy package BTW ;) )

Kubuntu 22.04 Emacs 28.2 Last version of doom-modeline on Melpa

Steps to reproduce

Expected behavior

Emacs not to freeze

OS

Linux

Emacs Version

28

Emacs Configurations

No response

Error callstack

No response

Anything else

No response

frederic-santos commented 1 year ago

I confirm that I have the same issue, and deactivating doom-modeline fixed it. (Strangely enough, I could reproduce this issue on a second computer with Emacs 28.2, but not on a third one with Emacs 29. If it can help...)

seagle0128 commented 1 year ago

I could not reproduce the issue. The backtraces would help for troubleshooting. Please try emacs --debug-init.

Two quick questions

  1. Does the issue occurs with emacs -Q (then use pacakge-initialize and enable doom-modeline-mode)?
  2. Are you using gccemacs or not?

So far, I think the workarounds are below. One of these should work.

  1. Delete doom-modeline from ~/.emacs.d/elpa/, and restart Emacs.
  2. (byte-recompile-directory "path/to/doom-modeline/" nil t).
frederic-santos commented 1 year ago

Thanks for the input!

For your quick questions:

  1. "Does the issue occurs with emacs -Q..." > Yep!
  2. gccemacs > Nope.

(@maxecharel , is it also the case for you?)

I'll have to investigate a bit further to try your suggestions (thanks again, btw!). I can't try right now, but will ping you back later. ;)

make-all commented 1 year ago

I have the same issue with freezing on Windows, but I do not seem to get the "function provided already compiled" message, so that may be a red herring. Some further info I have found: if I run the same config lines after Emacs is started, it appears to work, but the freeze reproduces if I create a new frame.

(use-package 'doom-modeline
  :ensure t
  :config
  (doom-modeline-mode 1))
maxecharel commented 1 year ago

@seagle0128 thx for feedback; concerning your questions

  1. "Does the issue occurs with emacs -Q > nope, so when I start it without the init file I am able to M-x package-initialize then M-x doom-modeline-mode and it works (so apparently not exactly the same issue as yours @frederic-santos). Note that I load doom-modeline with use-package in my init file. I will try to see whether a minimal config of it solves the issue at startup.
  2. gccemacs > no, my Emacs is installed with configure --with-xml2 make make install and that's it

Concerning the backtraces with emacs --debug-init, cannot access to the buffer since Emacs is completely stuck at startup; I guess there is a way to pipe the backtraces directly to a file, I will check that.

I will try the workarounds ASAP and will get back to you, thx again for your feedback @seagle0128

frederic-santos commented 1 year ago
  1. "Does the issue occurs with emacs -Q > nope, so when I start it without the init file I am able to M-x package-initialize then M-x doom-modeline-mode and it works (so apparently not exactly the same issue as yours @frederic-santos).

To be more precise, when doing this, I get the same thing as @make-all : everything does seem to work well, but Emacs freezes when I open a new frame, or do other specific actions. For instance, I have an instant freeze when I try M-x list-packages RET, in all cases.

But all of us seem to have slightly different behaviors... not very easy for @seagle0128 😓

samleeney commented 1 year ago

I am also having this issue after a package update today. For me, it appears to only occur when opening a .org file. It can also be temporarily fixed by resizing window (but only to some sizes, specifically when taking up 1/3 of the screen).

Commenting out modeline fixes it. All was working prior to update.

Edit: I have since rolled back to an older version of doomline and it is still breaking. This leads me to believe the issue is caused by some other commonly used package update breaking doomline rather than a commit in doomline itself.

felipebalbi commented 1 year ago

I just upgraded to the latest melpa version doom-modeline-20230106.1427 and I see the same behavior. Disabling doom-modeline from my init.el makes it work again. Enabling doom-modeline freezes all over the place. I noticed that opening a buffer for a mode managed by eglot is an easy way to see the problem as it's missing the eglot decorations on the modeline.

marcuskammer commented 1 year ago

I am having the same issue with version 20230106.1427. There is my minimal init file https://pastebin.com/pJs82g1n I called it with $ emacs -q -l tmp/emacs.el

The same behavior. Emacs is freezing. Emacs version 28.2 on Windows and Linux.

Tdback commented 1 year ago

I am also having the same issue with version 20230106.1427. It works when running emacs -q or if the doom-modeline package is commented out or deleted, but if the package is in my configuration Emacs will freeze after starting, and I have to force-close the client.

Emacs version 28.2 on OS X.

fredericgiquel commented 1 year ago

Same kind of problem for me with Emacs 28.2.

It seems to be related to the recent update of compat package that add Emacs 29 compatibility.

The problem can be highlight with the following minimal init.el using straight.el:

(defvar bootstrap-version)
(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el" user-emacs-directory))
      (bootstrap-version 6))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously
         "https://raw.githubusercontent.com/radian-software/straight.el/develop/install.el"
         'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  (load bootstrap-file nil 'nomessage))

(straight-use-package 'use-package)
(setq straight-use-package-by-default t)
 
(use-package doom-modeline
  :init (doom-modeline-mode 1)
  :custom (doom-modeline-height 30))

Changing doom-modeline-height is required to trigger the freeze.

With last version of all packages, Emacs freezes at startup. But with straight, we can easily rollback to an old version of a package. So let's do it for compat:

7ca7d300d1d256f674f83932d2918d8e70cd28f6 is the last version of compat before recent works to add Emacs 29 compatibility.

fredericgiquel commented 1 year ago

Commenting out the (require 'compat) line in doom-modeline-core.el file makes disappear the freeze at startup with the mininal init.el. But I still got problems with my complete configuration.

egelja commented 1 year ago

I have the same issue with freezing on Windows, but I do not seem to get the "function provided already compiled" message, so that may be a red herring. Some further info I have found: if I run the same config lines after Emacs is started, it appears to work, but the freeze reproduces if I create a new frame.

(use-package 'doom-modeline
  :ensure t
  :config
  (doom-modeline-mode 1))

I have this same issue on Windows too.

felipebalbi commented 1 year ago

Hi,

I have just bisected the issue down to this commit:

1d400aa88372962be6dad80182c244df3fe6e00b is the first bad commit
commit  
Author: Vincent Zhang <seagle0128@gmail.com>
Date:   Tue Jun 28 15:18:51 2022 +0800

    Fix #545: modeline is cutoff or has extra spaces at the right end depending on font size.

    NOTE: string-pixel-width brings both accurate calculation and negative performance impacts.

 doom-modeline-core.el | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Here's the full bisection log:

$ git bisect log
git bisect start
# status: waiting for both good and bad commits
# good: [0d6220900bbf27b4d2b0d95e820c3d6aa20ab253] Update FAQ.
git bisect good 0d6220900bbf27b4d2b0d95e820c3d6aa20ab253
# status: waiting for bad commit, 1 good commit known
# bad: [4224fcee7a6356e312169ccdbb156f380298a843] Fix modified face.
git bisect bad 4224fcee7a6356e312169ccdbb156f380298a843
# bad: [59821dbf918eb296ca94d9da9f841e07fc8c0dda] `buffer-local-value' is an obsolete generalized variable since 29.
git bisect bad 59821dbf918eb296ca94d9da9f841e07fc8c0dda
# bad: [f770e987418fc379466f1fe14eba7543386e32d6] Remove warnings.
git bisect bad f770e987418fc379466f1fe14eba7543386e32d6
# good: [87d766746a791c2ca6e5362338a7a4900c60400c] Silence the compiler
git bisect good 87d766746a791c2ca6e5362338a7a4900c60400c
# bad: [57444497fd8f50f47f18e9f9d595e4be9e2d3b4e] Revert "[Refactor] Remove the workaround for calculating icon's width."
git bisect bad 57444497fd8f50f47f18e9f9d595e4be9e2d3b4e
# bad: [1d400aa88372962be6dad80182c244df3fe6e00b] Fix #545: modeline is cutoff or has extra spaces at the right end depending on font size.
git bisect bad 1d400aa88372962be6dad80182c244df3fe6e00b
# good: [16679e21256754cdcf4c35535546a8aca5a2a295] Bump version to 3.3.2.
git bisect good 16679e21256754cdcf4c35535546a8aca5a2a295
# good: [4ac4ca91b4f307d51ed2f006c6c967c8af91f3ee] [Enhancement] Specify space with half width.
git bisect good 4ac4ca91b4f307d51ed2f006c6c967c8af91f3ee
# first bad commit: [1d400aa88372962be6dad80182c244df3fe6e00b] Fix #545: modeline is cutoff or has extra spaces at the right end depending on font size.

Attempting to revert that commit on today's HEAD results in a non-trivial conflict, but hopefully this gives enough information to reproduce and fix the issue.

I have also verified that hard resetting to the commit right before the offending commit results in a working setup.

As a workaround, we can all force doom-modeline to stay on v3.3.1 as that's the latest working tag on the repository.

UPDATE: This change makes the problem go away:

diff --git a/doom-modeline-core.el b/doom-modeline-core.el
index 8f5cc2864271..c2db56a3699a 100644
--- a/doom-modeline-core.el
+++ b/doom-modeline-core.el
@@ -1088,7 +1088,7 @@ Example:
                           (- (+ right right-fringe right-margin scroll-bar)
                              ,(let ((rhs-str (format-mode-line (cons "" rhs-forms))))
                                 (if (fboundp 'string-pixel-width)
-                                    (/ (string-pixel-width rhs-str)
+                                    (/ (string-width rhs-str)
                                        (doom-modeline--font-width)
                                        1.0)
                                   (* (string-width rhs-str)
felipebalbi commented 1 year ago

Oh, I see. string-pixel-width is introduced by emacs29 and compat provides a compatibility layer. Interestingly, string-pixel-width is marked as untested as can be seen here. Perhaps we should report a bug against compat?

felipebalbi commented 1 year ago

It seems like we could close this issue after compat gets a new release :-)

maxecharel commented 1 year ago

It seems like we could close this issue after compat gets a new release :-)

Perfect, thanks a lot @felipebalbi . I've seen that @minad directly handled the problematic function in compat, <3 Emacs community. Let's wait for the new release to be issued than we'll close this issue :)

seagle0128 commented 1 year ago

@felipebalbi You are right! The root cause is string-pixel-width from compat. The implement of string-pixel-width in compat has bad performance, like doom-modeline did before. See a1411d5 (which was reverted due to the performance issue).

IMO It's not easy to improve string-pixel-width. So I make a patch and commit. Hope it would address this issue before compat fixes it.

felipebalbi commented 1 year ago

IMO It's not easy to improve string-pixel-width. So I make a patch and commit. Hope it would address this issue before compat fixes it.

What if you explicitly check for the emacs version instead of checking for the existence of the string-pixel-width symbol? That way, you'll be certain that only emacs 29 uses string-pixel-width regardless of what compat is doing underneath.

In any case, minad's change is already removing the implementation from compat-29

minad commented 1 year ago

Thank you all. Generally my recommendation is to avoid using UNTESTED definitions from Compat just yet. Unfortunately there are still around 52 untested definitions there as of now. For comparison, 144 definitions have test coverage, which is ensured also by CI. In other words, as soon as a package author intends to use an UNTESTED function, a PR adding tests to Compat would be greatly appreciated.

In this particular case, doom-modeline is not to blame since it had a dynamic fboundp check, which got then defeated by Compat.

@seagle0128 In case you figure out a possible implementation of string-pixel-width I would like to put it back into Compat. ~The definition which was in Compat was copied verbatim from Emacs 29 itself, which then exposed an underlying bug in window-text-pixel-size I believe.~ (EDIT: I have to correct myself. The underlying buffer-text-pixel-size was copied from an earlier version from the shr.el library, with poor performance as pointed out by @seagle0128. See https://github.com/emacs-mirror/emacs/blob/ae9bfed50dbf5043c0b47f20473ef43d8aeebebd/lisp/net/shr.el#L659-L688)

@felipebalbi Yes, an explicit Emacs 29 check would have helped. I also considered that. However it is safer to not provide the broken and untested string-pixel-width function in Compat for now. Note that the function was just added in a recent release, so removing it again in a hotfix release won't do harm for downstream packages, since the function has not seen adoption yet. In the future I can hopefully refrain from such radical measures. New functions will only be added to Compat with sufficient test coverage.

felipebalbi commented 1 year ago

@seagle0128 In case you figure out a possible implementation of string-pixel-width I would like to put it back into Compat. The definition which was in Compat was copied verbatim from Emacs 29 itself, which then exposed an underlying bug in window-text-pixel-size I believe.

if there's confirmation of a bug in window-text-pixel-size, should we file a bug against emacs 28.2, in that case? Maybe diffing emacs 28.2 and emacs 29 version of window-text-pixel-size ought to give some hints of a fix that should be backported from emacs 29 branch, perhaps?

minad commented 1 year ago

@felipebalbi See my correction above. As @seagle0128 pointed out it may be very hard or even impossible to backport this function.

minad commented 1 year ago

@seagle0128 Just for me to understand fb516af4369043811f98929d7fbe2e43d0847242 - so you say that string-pixel-width did actually work correctly (considering computed values only) but was just prohibitively slow? It was a performance bug? Do you think we could mitigate that with caching?

seagle0128 commented 1 year ago

if there's confirmation of a bug in window-text-pixel-size, should we file a bug against emacs 28.2, in that case? Maybe diffing emacs 28.2 and emacs 29 version of window-text-pixel-size ought to give some hints of a fix that should be backported from emacs 29 branch, perhaps?

@felipebalbi buffer-text-pixel-size is implemented by C in 29, and it has good performance. If it calls window-text-pixel-size to implement, the performance is bad. I cannot say it's a bug of windows-text-pixel-size, because perhaps it's as designed. You should inquiry to Emacs maintainers. I remember I read some topics in mail list but I cannot find it now.

Anyway I check the emacs version in doom-modeline and the issue should go away now.

In 29

;;;###autoload
(defun string-pixel-width (string)
  "Return the width of STRING in pixels."
  (if (zerop (length string))
      0
    ;; Keeping a work buffer around is more efficient than creating a
    ;; new temporary buffer.
    (with-current-buffer (get-buffer-create " *string-pixel-width*")
      ;; `display-line-numbers-mode' is enabled in internal buffers
      ;; that breaks width calculation, so need to disable (bug#59311)
      (when (bound-and-true-p display-line-numbers-mode)
        (display-line-numbers-mode -1))
      (delete-region (point-min) (point-max))
      (insert string)
      (car (buffer-text-pixel-size nil nil t)))))
felipebalbi commented 1 year ago

Could we use the something similar to what transient is doing? Here it is reproduced:

(defun transient--pixel-width (string)
  (save-window-excursion
    (with-temp-buffer
      (insert string)
      (set-window-dedicated-p nil nil)
      (set-window-buffer nil (current-buffer))
      (car (window-text-pixel-size
            nil (line-beginning-position) (point))))))
seagle0128 commented 1 year ago

@seagle0128 Just for me to understand fb516af - so you say that string-pixel-width did actually work correctly (considering computed values only) but was just prohibitively slow? It was a performance bug? Do you think we could mitigate that with caching?

@minad Yes. In my testing, the behavior is correct, but the performance is poor. Caching should be helpful, but I don't think it's a good idea. The solution may be complicated, but you can try.

minad commented 1 year ago

@felipebalbi No, that is more or less what Compat used. Due to the window buffer switching and window-text-pixel-size it will be prohibitively slow for some use cases. I assume doom-modeline is calling that function often and in particular in the modeline code which is executed during redisplay. In the case of Transient the function is probably only called rarely.

seagle0128 commented 1 year ago

Could we use the something similar to what transient is doing? Here it is reproduced:

(defun transient--pixel-width (string)
  (save-window-excursion
    (with-temp-buffer
      (insert string)
      (set-window-dedicated-p nil nil)
      (set-window-buffer nil (current-buffer))
      (car (window-text-pixel-size
            nil (line-beginning-position) (point))))))

@felipebalbi If you look into a1411d5, you will find out the codes are same, and obviously I failed to backport.

felipebalbi commented 1 year ago

There has to be something else going on. If I define string-pixel-width after initialization (from scratch buffer, with a regular defun) then it works just fine.

Any easy way to extract the exact string doom-modeline is passing to string-pixel-width?

seagle0128 commented 1 year ago

@felipebalbi No, that is more or less what Compat used. Due to the window buffer switching and window-text-pixel-size it will be prohibitively slow for some use cases. I assume doom-modeline is calling that function often and in particular in the modeline code which is executed during redisplay. In the case of Transient the function is probably only called rarely.

@minad Correct. It depends on how often it's called.

felipebalbi commented 1 year ago

@minad I see now. The problem is that doom calls it on redisplay. Calling it once is not an issue. Understood :-)

minad commented 1 year ago

@felipebalbi I also assume that Emacs has a bad time to handle redisplay and set-window-buffer. This feels a bit like a redisplay in a redisplay. From my understanding the code executed during redisplay should be highly restricted. That is also the reason why I don't use fancy mode line packages in my Emacs setup. It feels simply too "dangerous" and error-prone to me. But I am sure @seagle0128 knows more about than I do and took great care in doom-modeline to avoid issues.

minad commented 1 year ago

Compat 29.1.1.0 has been released yesterday, which should fix this and related issues by removing the problematic string-pixel-width function again.

felipebalbi commented 1 year ago

Just updated both compat and doom modeline and I can confirm it works fine on my setup without modifications. Thanks @minad and @seagle0128 for providing fixes.

minad commented 1 year ago

@felipebalbi Thanks for confirming and thanks again for helping out with this issue!

maxecharel commented 1 year ago

Same as @felipebalbi; just updated both packages and now it works perfectly. Thanks @seagle0128, @minad and @felipebalbi for you work!

minad commented 1 year ago

@seagle0128 You were mistaken that buffer-text-pixel-size is not implemented based on window-text-pixel-size in Emacs 29. See the C source:

DEFUN ("buffer-text-pixel-size", Fbuffer_text_pixel_size, Sbuffer_text_pixel_size, 0, 4, 0,
       doc: /* Return size of whole text of BUFFER-OR-NAME in WINDOW.
BUFFER-OR-NAME must specify a live buffer or the name of a live buffer
and defaults to the current buffer.  WINDOW must be a live window and
defaults to the selected one.  The return value is a cons of the maximum
pixel-width of any text line and the pixel-height of all the text lines
of the buffer specified by BUFFER-OR-NAME.

The optional arguments X-LIMIT and Y-LIMIT have the same meaning as with
`window-text-pixel-size'.

Do not use this function if the buffer specified by BUFFER-OR-NAME is
already displayed in WINDOW.  `window-text-pixel-size' is cheaper in
that case because it does not have to temporarily show that buffer in
WINDOW.  */)
  (Lisp_Object buffer_or_name, Lisp_Object window, Lisp_Object x_limit,
   Lisp_Object y_limit)
{
  struct window *w = decode_live_window (window);
  struct buffer *b = (NILP (buffer_or_name)
              ? current_buffer
              : XBUFFER (Fget_buffer (buffer_or_name)));
  Lisp_Object buffer, value;
  specpdl_ref count = SPECPDL_INDEX ();

  XSETBUFFER (buffer, b);

  /* The unwind form of with_echo_area_buffer is what we need here to
     make WINDOW temporarily show our buffer.  */
  /* FIXME: Can we move this into the `if (!EQ (buffer, w->contents))`?  */
  record_unwind_protect (unwind_with_echo_area_buffer,
             with_echo_area_buffer_unwind_data (w));

  set_buffer_internal_1 (b);

  if (!EQ (buffer, w->contents))
    {
      wset_buffer (w, buffer);
      set_marker_both (w->pointm, buffer, BEG, BEG_BYTE);
      set_marker_both (w->old_pointm, buffer, BEG, BEG_BYTE);
    }

  value = window_text_pixel_size (window, Qnil, Qnil, x_limit, y_limit, Qnil,
                  Qnil);

  unbind_to (count, Qnil);

  return value;
}

string-pixel-width is then based on top of that:

(defun string-pixel-width (string)
  "Return the width of STRING in pixels."
  (if (zerop (length string))
      0
    ;; Keeping a work buffer around is more efficient than creating a
    ;; new temporary buffer.
    (with-current-buffer (get-buffer-create " *string-pixel-width*")
      ;; `display-line-numbers-mode' is enabled in internal buffers
      ;; that breaks width calculation, so need to disable (bug#59311)
      (when (bound-and-true-p display-line-numbers-mode)
        (display-line-numbers-mode -1))
      (delete-region (point-min) (point-max))
      (insert string)
      (car (buffer-text-pixel-size nil nil t)))))

Could you investigate what actually causes the problem here? Maybe we can backport string-pixel-width as part of Compat after all. Is the problem that save-window-excursion is too slow? I wonder why the function was moved to C in the first place.

seagle0128 commented 1 year ago

@minad I didn't say buffer-text-pixel-size was implemented based on window-text-pixel-size. I mean the alternative functions call window-text-pixel-size, and the performance is bad.

minad commented 1 year ago

@seagle0128 I don't understand. Do you think a backport is possible or not?

See what you wrote here: https://github.com/seagle0128/doom-modeline/issues/601#issuecomment-1374504718. This is incorrect. buffer-text-pixel-size IS implemented based on window-text-pixel-size in C.

seagle0128 commented 1 year ago

@minad It's possible if buffer-text-pixel-size or something like that is backported as well, I think. It needs to read the C codes carefully, but I didn't do that.

If it calls window-text-pixel-size to implement, the performance is bad.

Here "it" represents the implementation of string-pixel-width like this:

(defun doom-modeline--string-pixel-width (string)
  "Return STRING pixel width.
See `shr-pixel-column'."
  (if (fboundp 'string-pixel-width)
      (string-pixel-width string)
    (let ((pt (point)))
      (prog1
          (with-temp-buffer
            (insert string)
            (if (not (get-buffer-window (current-buffer)))
                (save-window-excursion
                  ;; Avoid errors if the selected window is a dedicated one,
                  ;; and they just want to insert a document into it.
                  (set-window-dedicated-p nil nil)
                  (set-window-buffer nil (current-buffer))
                  (car (window-text-pixel-size nil (line-beginning-position) (point))))
              (car (window-text-pixel-size nil (line-beginning-position) (point)))))
        (goto-char pt)))))