Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SVG] Main performance issue #369

Open
thierryvolpiatto opened this issue Jun 19, 2023 · 16 comments
Open

[SVG] Main performance issue #369

thierryvolpiatto opened this issue Jun 19, 2023 · 16 comments

Comments

@thierryvolpiatto
Copy link

Hello, following up from reddit (helm05).

I made all the changes to make Helm working with this all-the-icons branch.
The main issue I see is a loss of performance because the all-the-icons* functions return now a space with an svg image as display property.
Here from an helm-imenu function:
Capture d’écran_2023-06-19_07-49-23

Now printing the helm-imenu for all buffers (around 100) takes more than 30s whereas with master branch it is nearly instant.

@wyuenho
Copy link
Collaborator

wyuenho commented Jun 19, 2023

I'm not entirely sure why the difference is so high. Is it because you are running Emacs under WSL? If not, can you provide me with a profiler report?

@thierryvolpiatto
Copy link
Author

thierryvolpiatto commented Jun 20, 2023 via email

@thierryvolpiatto
Copy link
Author

thierryvolpiatto commented Jun 20, 2023 via email

@thierryvolpiatto
Copy link
Author

So no, this is not the reason, it is the repeated calls to locate-library which make things so slow, with this change, I get almost the same performances as before:

diff --git a/all-the-icons.el b/all-the-icons.el
index 671037e3b..7f78fd268 100644
--- a/all-the-icons.el
+++ b/all-the-icons.el
@@ -1100,6 +1100,7 @@ Return the icon file name if found."
                    (fn (all-the-icons--data-name (car mapping))))
           (assoc-default (cadr mapping) (funcall fn))))))
 
+(defvar all-the-icons-svg-library "/home/thierry/elisp/all-the-icons.el/svg/")
 (cl-defmacro all-the-icons-define-icon (name alist &key svg-path-finder (svg-doc-processor ''identity) (padding 0))
   "Macro to generate functions for inserting icons for icon set NAME.
 
@@ -1125,7 +1126,7 @@ PADDING is the number of pixels to be applied to the SVG image."
      (defun ,(all-the-icons--function-name name) (icon-name &rest args)
        (let* ((file-name (all-the-icons--resolve-icon-file-name icon-name ,alist (quote ,name))) ;; remap icons
               (size (window-default-font-height))
-              (lib-dir (concat (file-name-directory (locate-library "all-the-icons")) ,(format "svg/%s/" name)))
+              (lib-dir (concat all-the-icons-svg-library ,(format "%s/" name)))
               (image-path (concat lib-dir ,(or (and svg-path-finder
                                                     `(apply ,svg-path-finder file-name lib-dir size args))
                                                '(format "%s.svg" file-name))))

This is of course a dirty patch just to show where the slowdown happens.

@thierryvolpiatto
Copy link
Author

So probably computing the path of the svg directory one time for all is the way to go.

@thierryvolpiatto
Copy link
Author

So even if avoiding locate-library make things much faster, I still see some slowdowns due to stuff being computed at each of the function calls, probably these could be avoided or at least cached (memoized)?

@wyuenho
Copy link
Collaborator

wyuenho commented Jun 20, 2023

@thierryvolpiatto see if eb32540 works better.

About cache, the icons are cached at here. Caching them at the icon set level is going to blow up memory usage tremendously, and introduce more cache invalidation issues I have to deal with in addition to #367 and #368.

Let me know if you can think of other ways to speed up this branch :)

@thierryvolpiatto
Copy link
Author

thierryvolpiatto commented Jun 21, 2023 via email

@thierryvolpiatto
Copy link
Author

thierryvolpiatto commented Jun 21, 2023 via email

@thierryvolpiatto
Copy link
Author

thierryvolpiatto commented Jun 21, 2023 via email

@thierryvolpiatto
Copy link
Author

thierryvolpiatto commented Jun 22, 2023 via email

@thierryvolpiatto
Copy link
Author

For reference:
emacs-helm/helm@4a540f5

@thierryvolpiatto
Copy link
Author

Now helm is working fine with this branch, by using caching where needed I have reduced memory usage and increased speed.
When do you plan merging this branch to master?

@wyuenho
Copy link
Collaborator

wyuenho commented Jun 29, 2023

Don't know, whenever I have some time and brain space :) Feel free to send over a PR if you'd like

@thierryvolpiatto
Copy link
Author

thierryvolpiatto commented Jun 29, 2023 via email

@wyuenho
Copy link
Collaborator

wyuenho commented Oct 10, 2024

Just coming back to this issue now. I think this issue reflects a fundamental problem with the design of all-the-icons, namely it really tries to provide all the icons lol.

I think as a further improvement to this experiment, it's probably best to break up all-the-icons into some-file-icons, some-mode-icons, some-ui-icons, and some-core-icons. Basically, like how VSCode does it.

Cache busting is still a concern tho, I may have to advice custom-push-theme and all those set-face-* functions. This is gonna be a pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants