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

Add buffer-local caching to friendly-session calculation #3463

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vemv
Copy link
Member

@vemv vemv commented Sep 13, 2023

Friendly session calculation has always be at least a touch slow (ref, ref), and even more so following recent additions to it.

All slow paths have in common that they run seq-find over a series of sequences (the classpath, classpath roots, the ns list). Those can be arbitrarily large.

We may do relatively expensive stuff for each element, like trying to match a string.

So, this PR adds buffer-local caching.

The buffer-local caching is essentially this map:

<repl buffer> -> <is this buffer considered friendly to it?>

There are timestamps to ensure that we're not basing the caching on stale data.


From a quick test, it appears to work. Anyway I'll keep this as a draft to give it at least a couple days of QAing. Review welcome!

Seconds are too coarse-grained and can foil caching.
@@ -305,8 +305,15 @@ whether DIRECTION is 'from-nrepl or 'to-nrepl."
(seq-filter #'identity (mapcar f cider-path-translations))
(seq-some f cider-path-translations)))))

(defun cider--unix-time ()
"Returns the Unix time."
(float-time))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I'd get the time in nanoseconds, as integer. floats make me nervous :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with using (current-time)? Sure, it uses seconds, but I'm guessing that'd be fine for your use case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found out that seconds are too coarse-grained for timestamp comparisons.

Say that it was all computed in less than second, we'd get the same timestamp so > wouldn't succeed (and >= may not be as optimal to use)

@vemv
Copy link
Member Author

vemv commented Sep 17, 2023

I'll retake this PR after we get to address #3468 - otherwise not cool to further touch this area with a release around the corner

@bbatsov
Copy link
Member

bbatsov commented Oct 31, 2023

Might be a good time to revisit this for the next minor release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants