bmag / emacs-purpose

Manage Windows and Buffers According to Purposes
GNU General Public License v3.0
496 stars 23 forks source link

Large memory usage when combined with company #149

Closed Zulu-Inuoe closed 5 years ago

Zulu-Inuoe commented 5 years ago

I've been using purpose combined with helm and company without trouble, but after turning on the window-purpose-x extension, I've been seeing large slowdowns caused by frequent GC. Using the memory profiler, I saw that a major contributor was purpose.

I've been doing some tinkering with the code, primarily in purpose--buffer-purpose-name-regexp and purpose--buffer-purpose-mode, since they were eating up most of the memory in their calls to purpose--iter-hash.

I managed to remove those call altogether, to avoid consing up a list just to be thrown away (and save an extra gethash), but now I'm kind of stuck: From my profiler session, I found that maphash itself is for whatever reason using up 50% of the memory in that run (1 GB in this instance). I've been tracing through the emacs source to figure out why, but haven't had much luck.

One thing to maybe consider is to make use of alists rather than hash tables. In this instance, the built-in purposes is something like 65, and I doubt that user-provided or third-party purpose counts would ever reach anything even in the 100's. So from a performance perspective it should be neutral.

Zulu-Inuoe commented 5 years ago

These are my main changes so far:

(defun purpose--buffer-purpose-mode (buffer-or-name mode-conf)
  "Return the purpose of buffer BUFFER-OR-NAME, as determined by its
mode and MODE-CONF.
MODE-CONF is a hash table mapping modes to purposes."
  (when (get-buffer buffer-or-name)     ; check if buffer exists
    (cl-block nil
      (maphash
       (let ((major-mode (purpose--buffer-major-mode buffer-or-name)))
         #'(lambda (mode purpose)
             (when (provided-mode-derived-p major-mode mode)
               (cl-return purpose))))
       mode-conf))))

This makes use of a block and return rather than generating the list of derived modes and then filtering it.

(defun purpose--buffer-purpose-name-regexp (buffer-or-name regexp-conf)
  "Return the purpose of buffer BUFFER-OR-NAME, as determined by the
regexps matched by its name.
REGEXP-CONF is a hash table mapping name regexps to purposes."
  (cl-block nil
    (maphash
     #'(lambda (regexp purpose)
         (when (purpose--buffer-purpose-name-regexp-1 buffer-or-name
                                                      regexp
                                                      purpose)
           (cl-return purpose)))
     regexp-conf)))

likewise, prevents generating a list and filtering it

(defun purpose--iter-hash (function table)
  "Like `maphash', but return a list the results of calling FUNCTION
for each entry in hash-table TABLE."
  (let (results)
    (maphash #'(lambda (key value)
                 (push (funcall function key value) results))
             table)
    results))

iter-hash isn't being used any more, but I changed it so that it doesn't call append (which conses up a fresh list each call) and instead just uses push to build up the list

Zulu-Inuoe commented 5 years ago

By the way, I'd be happy to do the actual work of converting to alists if you'd like. I didn't want to do it without consulting first. I'm not going to bother trying to investigate why exactly maphash is causing that overhead.

Thanks!

bmag commented 5 years ago

Hey, thanks for opening all these issues and PRs. Sorry I'm late to respond, I'll try to evaluate and answer all of them one-by-one during the next several days.

Zulu-Inuoe commented 5 years ago

No worries. Just trying to take a little initiative instead of always complaining.

Thanks!

bmag commented 5 years ago

Regarding conversion from hash tables to alists, it will probably only make a small difference (which is still good). If you have the time for it, you are welcome to make the conversion, I don't foresee any problems with it. Coincidently, a long time ago I did a overhaul of the configuration in a separate branch for a future 2.0 version of Purpose (which might never happen), where I got rid of the hash tables. You can see it at https://github.com/bmag/emacs-purpose/tree/config-2.0.

Zulu-Inuoe commented 5 years ago

Ah cool. Well, I don't think the conversion to alists is necessary with the patches in #152 , since the consing is only an issue when purpose is getting triggered tons of times.