asok / projectile-rails

Emacs Rails mode based on projectile
258 stars 59 forks source link

projectile-rails-goto-file-at-point is really slow #102

Closed ejoubaud closed 8 years ago

ejoubaud commented 8 years ago

projectile-rails-goto-file-at-point is really slow, at least on a constant on a big project. On a fast MBP with SSD it hangs for seconds. The vim equivalent (vim-rails' gf) is instantaneous.

Looking at the code I think I may have narrowed it down to the (format ".*/%s\\.rb$" (projectile-rails-declassify name)) match in https://github.com/asok/projectile-rails/blob/master/projectile-rails.el#L973 That regex means it's looking through every dir in your Rails project right? If you have a lot of tmp files and public assets and local gem files (and boy do I), that's bound to be wildly inefficient. Perhaps a better formula might be to limit the search to (app|lib|config|engines)/.*/%s\\.rb$ (ideally even excluding app/assets).

Using projectile file search to leverage ignored dirs might also be an option (not that I have a good idea what I'm talking about).

If the reasoning holds I'll try and put up a PR but I'm a total noob in elisp so it might to take me a while and be ugly as hell.

Edit: My first diagnosis was wrong, projectile-rails-find-constant does filter by app dir, the problem is it lists every file in there:

(length (--mapcat (f-entries it #'f-file? t)
                        (-filter #'f-exists?
                                (-map #'projectile-rails-expand-root
                                        (projectile-rails--code-directories)))))

returns all 7k entries in my app/ subdirs. Just that part takes about one second to run, then (--filter (string-match-p ... runs a regex on all 7k entries. So far I can't find any good native lisp function for recursive file searching but there has to be a more efficient way, if only shelling out an ls app/**/#{filename}.rb

ejoubaud commented 8 years ago

Checking for the filename match directly on f-entries makes it ~4 times faster:

(benchmark-run 3 (--mapcat (f-entries it (lambda (n) (string-match-p "negate_purchase_job.rb" n)) t)
                        (-filter #'f-exists?
                                (-map #'projectile-rails-expand-root
                                        (projectile-rails--code-directories)))))

(1.9905990000000002 14 0.7250120000000138)

ELISP> (benchmark-run 3 (--filter (string-match-p ".*/negate_purchase_job\\.rb$" it)
        (-uniq
        (--mapcat (f-entries it #'f-file? t)
                    (-filter #'f-exists?
                            (-map #'projectile-rails-expand-root
                                    (projectile-rails--code-directories)))))))

(6.981109 15 0.754205000000006)
ejoubaud commented 8 years ago

Another option that would make it much much faster would be to give up on recursive search and look for the whole module name (including namespaces) in app/*/#{contant_file_}.

That would break implicit local namespaces. A clever upward traversal logic could replace it, it would still be much faster but likely hard to write...

ejoubaud commented 8 years ago

Replaced the choices block with this, if things did get faster it's not noticeably so, and still a long shot from snappy:

(let ((file-name (format "/%s\\.rb$" (projectile-rails-declassify name)))
      (code-dirs (-filter #'f-exists? (-map #'projectile-rails-expand-root (projectile-rails--code-directories)))))
     (-uniq (--mapcat (f--entries it (string-match-p file-name it) t) code-dirs)))))

It seems most of the improvement in the benchmarks was due to replacing the regex ".*/%s\\.rb$" with "/%s\\.rb$".

Let me know if you reckon it's worth a PR anyway or if you have any thoughts/other optimization ideas to benchmarks.