Ticket #3247 (closed defect: fixed)

Opened 3 years ago

Last modified 2 years ago

status_msg_init's delay of 1 sec is way too large

Reported by: egmont Owned by: andrew_b
Priority: major Milestone: 4.8.14
Component: mc-core Version: 4.8.13
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

Followup of #2136:

status_msg_init() is always called with a delay of 1 second. This delay is very bad, it's a terrible, frustrating user experience.

This means that an action initiated by the user (e.g. press Ctrl+space on a large directory to compute its recursive size, or start copying a large directory tree) often doesn't have any visible feedback for this long. When it comes to usability and UI responsiveness, 1 second is much larger than okay.

1 second is way too large for you to start wondering whether you didn't hit the particular button hard enough, or hit the wrong key, or the wrong window is focused, or something else went wrong. You start looking around hunting the cause, or maybe even reach out with your fingers to press Enter again, or something like this. Actually, 1 second is large enough to press the key again which might have unexpected consequences. Or you just sit there wondering why mc is so goddamn slow (it is not, it just makes you believe this.)

Generally, any keypress should have a much quicker, preferably immediate feedback to let the user know that mc has indeed received the keypress and is doing its job.

It is even worse when a previous dialog is about to disappear (e.g. you start copying by pressing Enter in the F5 dialog), here at least the old dialog should disappear immediately rather than with a delay.

I think that around 0.1 sec (which is 2-3 frames with the standard 24 FPS) would be okay for such a delay, it's probably soon enough that you don't begin wondering why mc didn't act on your keypress, yet avoids a flicker if the operation completes quickly.

(Note: you cannot avoid all the flickers: no matter when you present the popup dialog, it's always possible that the operation completes just after that.)

I would personally totally remove this delay. It just causes more complicated code and worse user experience. I'm not sure about the internals of terminal emulators in general, but I do know VTE (gnome-terminal) quite well, and this one definitely doesn't update its display immediately, it has a rate limit for its updates. I'm pretty sure it's the same for most (all?) other terminal emulators, otherwise they'd be slow as hell. This means that if mc draws a dialog, completes its action really quickly, and finally removes the dialog, most likely the dialog never makes it into actual X11 pixels. Such a buffering layer under mc pretty much makes status_msg_init()'s delay pointless.

Even if you decide not to remove the asynchronous behavior and not to simplify the code, please at least significantly decrease the timeout, maybe to ~0.1 seconds or so (and please make it a #define rather than repeating this value all over the code).

Attachments

mc-3247-directory-size-frame-limit-PoC.patch (3.5 KB) - added by egmont 3 years ago.
Proof of concept

Change History

comment:1 in reply to: ↑ description ; follow-up: ↓ 2 Changed 3 years ago by andrew_b

Replying to egmont:

status_msg_init() is always called with a delay of 1 second. This delay is very bad, it's a terrible, frustrating user experience.

I don't think so. One second is a nice compromise between speed of operation and user notification.
For reference, you can measure yourself the duration of directory size calculation (especially for directory with huge number of files, /usr, for example) for several cases:

  • dialog with calculation progress is raised immediately after start;
  • dialog is raised after 1 s
  • dialog is raised after some delay in the (0...1) second interval.

comment:2 in reply to: ↑ 1 Changed 3 years ago by egmont

Replying to andrew_b:

I don't think so. One second is a nice compromise between speed of operation and user notification.
For reference, you can measure yourself the duration of directory size calculation (especially for directory with huge number of files, /usr, for example) for several cases:

Do you have metrics how long these operations take for you in the three cases?

If you're scanning a tree as large as /usr, I'm pretty sure there's no measurable difference between delaying the UI by a second or not. If there is a difference then the problem is that the dialog is updated way too frequently and we could rate limit that to only a few frames per sec, but it's no reason for delaying it by 1 sec.

Last edited 3 years ago by egmont (previous) (diff)

comment:3 Changed 3 years ago by egmont

I compared 4.8.12 and 4.8.13 (release candidate), hitting Ctrl+Space on /usr (everything already being in the kernel's cache, not reaching out to my HDD). Time measurement is quite inaccurate (that is, me counting in my head :))

mc-4.8.12: Operation takes approx 5 seconds. After about 2 seconds the counter is at 10000 files, and altogether there are about 30000 files.

mc-4.8.13: Operation takes approx 4 seconds. After 1 second when the dialog appears, the counter is already at approx 10000.

Your observation that the operation becomes faster is correct. But using it to defend that delaying the popup the right thing to do is incorrect. The difference is fixed, if the operation took 100 seconds to complete in 4.8.12, it'd take 99.x seconds to complete in 4.8.13. The numbers clearly show that the operation becomes way faster during the first second due to the lack of the popup. But then why apply this speedup to the first 1 second only and not the rest of the operation? Why not omit the popup altogether to make it even much faster? (I tested it: increased the dialog delay to a giant value, now scanning /usr completes in about 3 seconds.)

The numbers clearly show that a significant portion, roughly around 40% of the time is spent by UI updates rather than actual work. The correct solution would be to rate limit the UI updates so that resources spent doing that become negligible. That would make the total operation complete in about 3 seconds in this example, even with the dialog popping up immediately.

comment:4 Changed 3 years ago by egmont

Note: rate limiting would not only make mc itself faster but also give significantly less work to the terminal emulator (which in turn frees up even more CPU time for mc), and probably fewer context switches will be required between the two. Or, if run over network, the network traffic would be heavily reduced.

Last edited 3 years ago by egmont (previous) (diff)

comment:5 Changed 3 years ago by egmont

8+ year old bug #43 also points out the same core problem. Probably time to address that, rather than a workaround that hardly increases performance, yet pays the price of a terribly unresponsive user experience.

Changed 3 years ago by egmont

Proof of concept

comment:6 Changed 3 years ago by egmont

I attached a proof of concept to prove my point. For the directory size calculation, I limited the display update to 25 FPS, which looks exactly as continuous to the eye as the previous version. The operation completes almost twice as fast as in 4.8.13, even though the popup is displayed almost immediately.

I hope this convinces everyone that this is the right approach, and that this is not hard to do at all.

(Note that if I decrease the delay any further, below the current magic threshold when the code decides to display it immediately, then the box's contents overflow out of the box. That is, the actual internal layout/behavior is different based on whether a non-negligible delay is requested or not. This is definitely a bug.)

comment:7 Changed 3 years ago by andrew_b

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Version changed from master to 4.8.13
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.14

Egmont, I implemented your proof of concept with my modifications.

Branch: 3247_status_msg_update_rate.
Initial changeset:6abea7e80afdde422f2c7bbb4659c4418d6078e1

I guess, the delay before directory scanning status is most critical place for you. Ok, now this delay is absent.
In other places where status_msg_t is used:

  • load file in editor
  • search in editor
  • autocomletion collect in editor
  • search in viewer

I would like to keep initial 1-second delay. In most cases, those action are fast (less than 1 s), and status show is pointless.

comment:8 Changed 3 years ago by egmont

I'm planning to continue working on this, and also address #43 for 4.8.14. I'll continue on top of your branch. There should be a rate limit on UI feedback of copy/move/remove operations (including their preparation when the directory tree is being scanned), probably a few more too. I'd also use timer in the rotating dash.

I'm generally firmly against any intentional delay. It was just coincidental that I mentioned directory scanning first and we're stuck with that. Delaying the feedback, no matter where it happens, gives the false idea that mc is slow, and that you might not have pressed the key you intended to press. E.g. when copying files, removal of the copy dialog should happen immediately. I see absolutely no point in delaying the popup, and it also complicates the code.

If the operation completes under 1s most of the time, you won't be able to read what's in the popup (but it's the same if you delay it by 1s and the operation takes 1.1s). But purely the appearing colored box in the middle of the screen immediately tells you that mc is working on your request, and that's already a lot better than nothing for a second.

As I'll go along with the patch, it might happen that I find a place where I think a delay is okay. I can't see that in advance. I'll experiment at some places, like search in viewer.

comment:9 Changed 2 years ago by slavazanko

  • Votes for changeset set to slavazanko

comment:10 Changed 2 years ago by angel_il

  • Votes for changeset changed from slavazanko to slavazanko angel_il
  • Branch state changed from on review to approved

comment:11 Changed 2 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from slavazanko angel_il to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [4addae7f12ac224297433610de6fcf6546da5ef4].

git log --pretty=oneline 99dd2ef..4addae7

comment:12 Changed 2 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.