Performance Problems for magithub-issue-insert-section #152
Comments
|
Can I ask how exactly you got that output? I used to know, but I can't seem to place my finger on the function. We might try something like magit-log does: if there are going to be many child sections, have a 'more' button that will load the remaining ones. (We can set the section-data to the remaining sections or even a form to execute to do the job.) Otherwise I made the change you suggested and don't notice much of an improvement: (defun magithub-issue-detail-insert-body-preview (issue fmt)
"Insert a preview of ISSUE's body using FMT."
(let-alist issue
(let* ((label-string (format fmt "Preview:"))
(label-len (length label-string))
(prefix (make-string label-len ?\ ))
did-cut)
(insert label-string
(if (or (null .body) (string= .body ""))
(concat (propertize "none" 'face 'magit-dimmed))
(concat
(s-trim
(with-temp-buffer
(insert (replace-regexp-in-string "^M" "" .body)) ;; todo: use actual control character
(goto-char (* 3 (- fill-column label-len)))
(forward-line 3)
(unless (eobp)
(setq did-cut t)
(backward-char 3))
(delete-region (point) (point-max))
(let ((text (buffer-string)))
(erase-buffer)
(insert (s-word-wrap (- fill-column label-len) text)))
(goto-char 0)
(let ((lines 0))
(while (and (not (eobp))
(< (setq lines (1+ lines)) 4))
(insert prefix)
(forward-line))
(unless (= (point) (point-max))
(delete-region (point) (point-max))
(setq did-cut t)))
(when did-cut
(propertize "..." 'face 'magit-dimmed))))
"\n")))) |
|
Ah, that's the output of the builtin emacs profiler, see
That would be really neat! After looking at this a bit, I'm starting to think the majority of the lag is coming from inserting magit entries, so paginating the log list would help a lot if that's the case. I'm probably still going to see if I can get some more concrete information, because it is very nice to have all the issues in one drawer. I'm going to try profiling that implementation of the function when I get some free time, but at a first glance I don't like it because it:
I don't know too much about emacs performance, so I could be wrong about those though (I'll let you know what I find out). Just a heads up about profiling, don't compare byte-compiled code to evaluated code (I made the mistake of evaluating my modifications, and have them always be slower). |
|
Here's an updated definition based on your comments. It's a little wordier, but hopefully faster. (defun magithub-issue-detail-insert-body-preview (issue fmt)
"Insert a preview of ISSUE's body using FMT."
(let-alist issue
(let* ((label-string (format fmt "Preview:"))
(label-len (length label-string))
(prefix (make-string label-len ?\ ))
(width (- fill-column label-len))
(maxchar (* 3 width))
(did-cut (< maxchar (length .body)))
(maxchar (if did-cut (- maxchar 3) maxchar))
(text (if did-cut (substring .body 0 maxchar) .body))
(text (replace-regexp-in-string "^M" "" text))
(text (s-word-wrap width (s-trim text))))
(insert label-string
(if (or (null .body) (string= .body ""))
(concat (propertize "none" 'face 'magit-dimmed))
(concat
(s-trim
(with-temp-buffer
(insert text)
(goto-char 0)
(forward-line)
(let ((lines 1))
(while (and (not (eobp))
(< (setq lines (1+ lines)) 4))
(insert prefix)
(forward-line))
(unless (eobp)
(delete-region (point) (point-max))
(setq did-cut t)))
(buffer-string)))
(when did-cut
(propertize "..." 'face 'magit-dimmed))))
"\n"))))I'll try to get the profiler up and running. |
|
Yup, that definition seems a little bit faster to me (when comparing the previous definition)! I still have no idea why Also, when profiling, you have to run |
|
I'm guessing the profiler isn't smart enough to descend into the evaluation of the letbound forms. I'll see what I can do to figure how long those take. |
|
I spent a bit of time working on this and I think the issue is in the usage of
Using the alias to surround suspcious parts helps pin down which forms are taking the most time. I found that something like: is much slower than inserting the entire text at once. Is it possible to build a string and then insert it, perhaps? (I'm not sure if string building is slower or not) I'm not too familiar with magit. If it isn't I think that it's probably best to limit the amount of stuff put into the magit buffer (my status buffer is over 3000 lines long). |
|
I didn't find concatenating the string before insertion to be effective on my end – we might be hitting a low-level difference in our respective computers. One workaround for you for now is to use @tarsius Do you recall hitting a threshold like this with |
No.
I would expect that concatenating first would be less efficient. Edit: but thinking about it again - maybe not. Performing some benchmarks and/or asking Eli seems like a good idea. |
|
I ended up solving this issue by filtering issues shown by magithub-status using this function: I think that there's probably nothing more we can do (from what I can see) so I'm fine with this being closed. Maybe it would be nice to provide sample filter functions to filter based on the current user (pulled from github token) but I don't mind if that's done or not. |
|
I'm a fan of some default filter functions. As a note to self, I'd want some way to advertise them in the customize interface. Small note to make your |
I always noticed on semi-large repostiories, magithub would slow down magit-status a lot. After seeing this in offline mode, I realized this is actually mostly performance problems, and not waiting for api calls.
I took a stacktrace of magit-status called repeatedly for
qutebrowser/qutebrowser(with 400 issues, and about 20 prs)`It turns out 75% of time is spent generating the details for issues (which I feel can be optimized down).
Right away I saw a possible improvement where we can trim the string description before wrapping it (rather than after), as 12% of time is spent wrapping long descriptions (that are never used later)
run-hook-with-argsseems to be taking a huge chunk of time itself, I'm not entirely sure why. I'm going to play with making that hook a function instead, and seeing if that improves performance (or gives me a better profile to look at).I'm more than happy to work on this, because I like this kind of thing a lot :). It might be a while before I can work on it though.
The text was updated successfully, but these errors were encountered: