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

A super hacky attempt at using $kak_command_fifo instead of a socket connection #15

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Screwtapello
Copy link

This was my exploration for #14.

In practice it's a lot choppier than the original implementation, but using $kak_response_fifo to keep things in lockstep means scrolling still completes in the same (wall clock) time, just at a lower frame rate.

@caksoylar
Copy link
Owner

Thanks, this looks like it simplifies a lot of code! Let me know if you have further comments on the long term experience; I will start using it as well to see how it performs.

"""
)
with open(self.response_fifo, "r") as handle:
handle.read()
Copy link
Owner

Choose a reason for hiding this comment

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

One question here since I don't grok response fifo very well: Is it strictly necessary to have kak write to the response fifo and us read it for this to work, or is it just to make sure kak finished processing before we send another event? If it is the latter, would the throughput improve if we remove it?

Copy link
Author

Choose a reason for hiding this comment

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

The plugin has a "duration" option, but unless there's some way for us to measure how long Kakoune spends doing stuff, we're only measuring the duration of sending messages, not the duration of the actual animation. Telling Kakoune to write to $kak_response_fifo after processing the message, and waiting for it to do so, gives us the timing information we need.

Deleting it slowed things way down for me - instead of drawing two or three frames over the intended quarter-second, it drew a smoother 5-10 frames over a full second.

Copy link
Owner

Choose a reason for hiding this comment

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

I see the point of getting more accurate timing information about the actual change; I assume below is what is happening if Kakoune cannot process the command inside interval duration:

  1. With response fifo, we will correctly tell that it took longer than the interval and wait until we send the next event, regardless of interval. I assume the max_duration condition is triggered after a few scroll events, so it will finish in a quarter second by jumping to the end.
  2. Without response fifo, we might underestimate run time and still sleep, probably until interval is elapsed, but send a new event immediately after it is elapse. We might end up just sending all scroll events one-by-one and never trigger max_duration. So we queue up many scroll events instead and Kakoune gets backed up and takes time to finish up the queue.

In hindsight this issue is present in the original implementation as well, but I guess the scroll event is just faster to complete Kakoune-side? I don't know if writing to response fifo would slow it down much, or <c-l> is costlier than the regular screen redraw Kakoune makes on a line scroll. I can try to benchmark the cost of these Kakoune-side and see if it is much significantly than just executing exec jvj.

Another idea: Since in this case we can know how long a scroll event exactly takes, we could actually try to play catch up by sending intermediate scroll events for multiple lines at once, when we determine we are behind timingwise. The bookkeeping to do that doesn't seem very trivial though.

Copy link
Author

Choose a reason for hiding this comment

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

I should note I've been testing on a tiny, slow netbook, so any performance problems would be magnified. The original "blast keys at the socket" implementation is actually pretty smooth, but the implementation in this PR is basically unusable (one of the reasons it's a draft PR).

vk probably just redraws a single line, while <c-l> redraws the entire screen so it can recover from "the screen isn't in the state Kakoune thinks it's in". This can be a significant difference on a big terminal, but my little laptop only has a 720p display so I'm not using a very big terminal, I'd be a little surprised if this were a big issue.

Animation timing is a whole thing, yeah. If you're lucky enough that drawing a frame is reliably faster than the delay between frames, then yeah, you don't have to worry too much. On the other hand, if you can't reliably meet your target frame rate, you need to structure your calculation so that the "time" unit is seconds, not frames. Once that's done, it shouldn't be too complex to say "at time t we should have scrolled 7 lines, so far we have scrolled 2, therefore we need to scroll 7-2 = 5 additional lines"

Copy link
Owner

Choose a reason for hiding this comment

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

On my relatively beefy desktop I didn't see too much performance difference, but I see some overshooting with page scroll keys which shouldn't really be caused by this change (unless there is some issue with Kakoune's command fifo processing). I think I'll need to do some debugging and see what's going on :/

Right, if we restructure the code by precomputing the cumulative intervals and storing them, then looking it up at every step your suggestion should work for catching up.

By the way it might help for you to bump up the SEND_INTERVAL constant on a slower machine, so that at the very least the initial lines would be grouped up in larger batches. It doesn't really help with catching up once behind but at least it would make it harder to fall behind.

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