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

Large memory usage when combined with company #149

Closed
Zulu-Inuoe opened this issue Jun 6, 2019 · 6 comments · Fixed by #152
Closed

Large memory usage when combined with company #149

Zulu-Inuoe opened this issue Jun 6, 2019 · 6 comments · Fixed by #152

Comments

@Zulu-Inuoe
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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!

Zulu-Inuoe added a commit to Zulu-Inuoe/emacs-purpose that referenced this issue Jun 17, 2019
Fixes bmag#149
The problem is that 'gathering a list of other buffers with the same purpose' conses due to an inevitable maphash later on, and  even though it's not a ton, it adds up quick because this function is called on every kill-buffer call. Even for the tons and tons of background temp buffers used by company, helm, sly, and others.

The fix: We don't need to figure out the list of other buffers unless we actually come across a window that was displaying the killed buffer. So this fix just delays calculating it until we actually need it.
Zulu-Inuoe added a commit to Zulu-Inuoe/emacs-purpose that referenced this issue Jun 17, 2019
Fixes bmag#149
The problem is that 'gathering a list of other buffers with the same purpose' conses due to an inevitable maphash later on, and  even though it's not a ton, it adds up quick because this function is called on every kill-buffer call. Even for the tons and tons of background temp buffers used by company, helm, sly, and others.

The fix: We don't need to figure out the list of other buffers unless we actually come across a window that was displaying the killed buffer. So this fix just delays calculating it until we actually need it.
@bmag
Copy link
Owner

bmag commented Jun 23, 2019

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
Copy link
Contributor Author

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

Thanks!

@bmag
Copy link
Owner

bmag commented Jun 28, 2019

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
Copy link
Contributor Author

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.

@bmag bmag closed this as completed in #152 Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants