Add cache project-crumbs #18
Comments
|
Reproducible benchmarks or it didn't happen :)
…On Sat, Sep 30, 2023, 12:52 roife ***@***.***> wrote:
I noticed that the breadcrumb--project-crumbs-1 function is being called
frequently, which leading to performance overhead. However, the
project-crumbs does not change frequently, so how about caching it to
optimize the performance?
—
Reply to this email directly, view it on GitHub
<#18>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC6PQ4O4UE2UYNOFNJDPTTX5ABW7ANCNFSM6AAAAAA5NQCHXQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Well, I opened |
|
Can't reproduce: Now, select "cpu", scroll down to line 300 more or less, then I get this: |
|
This was with Emacs 30. I can reproduce your problem with Emacs 28 though. It seems that your I think you have to restart to ensure the newer Anyway, when you do upgrade, the problem goes away. No need to duplicate the caching in breadcrumb. Let's keep it lower down where it can be useful to many other packages. |
|
Thank you for your contribution! I will consider upgrading my Emacs version first. |
|
A bit more information:
Following the profile report above: > emacs -Q -l breadcrumb.el -f breadcrumb-mode /nix/store/wq7b9287kw2403mk52qllcml6jqz8rbk-emacs-gtk3-29.1/share/emacs/29.1/lisp/jka-compr.el.gz -f profiler-startThe file being edited is problematic because one of the directores above it in the directory tree ( My current fix is to advise the function |
In that case, you do seem to be running the latest Because when In any case, this seems much more like a |
|
@alastairdb Could you try this patch? diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index aa92a73336e..2e1b4d96e4a 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -556,7 +556,7 @@ project-try-vc
;; Maybe limit count to 100 when we can drop Emacs < 28.
(setq last-matches
(condition-case nil
- (directory-files d nil marker-re t)
+ (directory-files d nil marker-re t 100)
(file-missing nil))))))
(backend
(cl-find-ifIt requires Emacs 28+, that's why I originally dropped this optimization. But it would be useful to know whether it really helps in your case, or if some other solution is needed. |
|
Yes, thanks a lot. The code is still spending a lot of time in Some other figures:
It seems that when a project is encountered in a directory, this is recorded via |
That's great!
Basically, because we don't have a solid cache invalidation strategy yet. Nobody notices that because removing project markers (deleting a repo, etc) is a very rare even, whereas creating one is something everybody does on an occasion (e.g. with For now, I will check in (EDIT: a version of) that patch (which should take care of the most egregious slowdown) and perhaps recommend that the clients of project.el sensitive to such low latency do some caching on their own. Even caching the current project for the buffer for 1 second should make this overhead go away, if the scrolling seems smooth enough for you already. |
|
Hmm, could you verify, though, that the patch really makes a difference? That is, that the call The reason I felt safe to skip this limit originally, is because the call is supposed to only return the files whose names match |
|
@dgutov I think you are correct. |
|
But you said that scrolling is smooth now? Did that change due to some other reason, or did you miss the failing scenario last time? If you simply counted the number of calls (but they are in practice fast enough), then I think I'll just recommend the second step: that |
Why doesn't project add this? |
Yes. I was a bit too hasty in my assessment. If you scroll in lots of small increments the delay is not so noticeable. That's why I ended up calling |
Like I said: Improving that is TBD. But an optimal caching strategy can depend on the application, so it's not a problem when an application does some caching. |
|
It's project.el who knows how much work it takes to request a project, and it's project.el who knows about it's limitations, so it should be project.el to know do whatever (optimal/suboptimal) caching it can. This seems clear to me. Then remove it with the TBD happens. |
|
A solution in project.el should be application-independent. So, pretty much perfect. That's harder than doing a single application-specific strategy. |
|
I just think it's odd for a package that knows its limitations not to be the one addressing them. Maybe not all backends require this caching or face this limitation. Maybe the limitation is worse for remote files. Eglot also uses a lot of project-current btw. Anyway, your call, I'll give you commit rights here and you can hack this in breadcrumb.el if you want. |
This really makes no sense to me. Like, right now, there is no solution. So there is 100% imperfection, 0%perfection. You'll only jump for 0% to 100% directly, no middle ground? |
|
The current situation is a middle ground: it does some caching. We could review some other middle-ground too, but it probably should have a better argument than "one application benefits from it".
Thank you for the invitation, I'll see if I find the time. |
As far as I understand, everyone calling project-current more than once for a given project-less buffer with will benefit. I presume you introduced the existing caching before breadcrumb.el was born, so whoever you were benefiting is also suffering in "no project" scenarios. Maybe the suffering is not in terms of freezing Emacs like it is here, but it's probably something. |
|
@dgutov I can confirm that project-0.11.1 (with upgraded xref) works much better. Thank you for this additional work. One thing which I'd like to consider is for breadcrumb to have an option to inhibit project and another to inhibit file name entirely on the breadcrumb line. This is often duplicative to information on frame title, mode-line, tab name, etc. I could submit a PR for that if any interest or perhaps this is already underway. If enabled, I might not even have noticed the non-imenu performance issues. My cache used buffer-local variables vs. project dir properties. I was able to leverage the locals for other reasons and avoid constantly calling I have an alternative project find function that goes first in The reason I have that function is that we use a variety of project configurations with the more complex ones in a large monorepo hierarchy and where independent, discrete subprojects are convenient to identify; e.g., web front end vs. server back end. Using private markers such as "venv" (for python) or ".project" (custom marker) allow us to do this. I have a "super root" marker, tagged as ".project.root", that allows tooling to identify non-vc master project roots vs subproject roots, tagged as ".project" tag; e.g., documentation files that do not live in vc but for which
I mention this use case FYI so future |
I don't understand what the slowdown was (did you read the "reproducible benchmarks or it didn't happen"? It still goes for whatever you are trying to communicate in #38). But all the same, good news and cheers to @dgutov whatever you did.
A valid concern, surely. But completely outside the scope of this issue -- which deals (or dealt) specifically with the slowdown introduced by the fact that project.el doesn't (didn't?) cache the absence of a project for a given file (because that cache is hard to invalidate reliably). And thus, in that particular case, breadcrumb performance would suffer substantially. So there was the idea to introduce an expiring cache in breadcrumb.el (or in project.el). That idea hasn't materialized yet, and maybe it doesn't need to For these "duplicate information" concerns, see the README for how to build your own breadcrumbish modeline constructs. Open a new issue/conversation if that's not enough. |
I'm not sure what that was, actually. Perhaps the fix for https://debbugs.gnu.org/69740? Otherwise it depends on which version the upgrade was from. And the version of xref shouldn't matter. I mean, there were some significant improvements for project-files recently (in large repos), but this package doesn't call
Without going into too much detail (which I'd be happy to address somewhere else, but using this bug tracker seems like an imposition), a non-vc master project is indeed a use case which the current features don't cover, and a custom |
I noticed that the
breadcrumb--project-crumbs-1function is being called frequently, which leading to performance overhead. However, the project-crumbs does not change frequently, so how about caching it to optimize the performance?The text was updated successfully, but these errors were encountered: