bugGNU nano - Bugs: bug #51491, any cursor movement on a huge...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #51491: any cursor movement on a huge softwrapped line is slow

Submitter:  Benno Schulenberg <bens>
Submitted:  Mon 17 Jul 2017 11:55:56 AM UTC
   
 
Severity:  2 - Minor Status:  Wont Fix
Assigned to:  None Open/Closed:  Closed

Jump to the original submission

Sun 01 Dec 2019 04:17:55 PM UTC, comment #19: 

The various optimizations to the character-parsing routines have brought considerable speedup for plain-ASCII texts.  Until an actual user complains about slowness with a real-world file, I'm going to let this be.

Benno Schulenberg <bens>
Group administrator
Fri 13 Jul 2018 10:43:07 AM UTC, comment #18: 

Commit 9a7ba5db is not a true fix, of course.  Things would be fixed if moving the cursor anywhere in a huge softwrapped line would always take the same amount of time.  But that is not the case, and would require enormous changes to achieve.  So... letting it be for now, and downscaling the importance, as the named commit has improved things quite a bit.

Benno Schulenberg <bens>
Group administrator
Tue 05 Jun 2018 07:15:18 PM UTC, comment #17: 

I haven't done official measurements either, but I have tested current git with an ASCII file consisting of one line of almost 50k (both before and after the commit you mention), and it's noticeably faster after the commit.

David Lawrence Ramsey <dolorous>
Group Member
Mon 04 Jun 2018 12:19:07 PM UTC, comment #16: 

Forgot to say: the first hunk in that commit speeds up things by roughly a factor of two.  The second hunk seems to have the most effect: an additional factor of three or four or five -- I haven't done any actual measurements.

Benno Schulenberg <bens>
Group administrator
Mon 04 Jun 2018 12:15:09 PM UTC, comment #15: 

Commit 9a7ba5db improves this significantly.  To verify, make your terminal at least 32 rows high and 135 coluns wide, and then run the following command, both for nano 2.9.8 (or older) and for current git:

  nano --ignore --soft --smooth +-1 src/nano

That puts the cursor on the last line of a binary file.  Then slowly (in the older nano) type <Up> until the cursor is on the top line of the edit window.  Then continue, very slowly, to type <Up>.  For me, it takes more than a second for the screen to scroll up one row (with the older nano).  With current git, the movement is nearly smooth -- there is just a slight delay.

Only when the cursor is beyond column twenty thousand in a line, do things start to become annoyingly slow.  I think that is acceptable.  So... I consider this bug mostly "fixed".

Benno Schulenberg <bens>
Group administrator
Sat 21 Apr 2018 11:03:10 AM UTC, comment #14: 

Gnulib's wcwidth() is only used because you are on a Mac, where the system's wcwidth() has some obscure bug.  On most systems, glibc is present and the system's wcwidth() is fine and is used.

The "slowness" that we see -- at the tail of softwrapped lines that are thousands of characters long -- happens also with the system's wcwidth().

But future versions of gnulib will have a somewhat faster wcwidth(): a mitigation was applied upstream: http://lists.gnu.org/archive/html/bug-gnulib/2018-04/msg00058.html.

Benno Schulenberg <bens>
Group administrator
Wed 18 Apr 2018 09:34:48 PM UTC, comment #13: 

I found by profiling that the real issue here is GNULib's wcwidth implementation.

It calls local_charset(), which calls the expensive strlen three times each in a for loop. It is the main bottleneck by far.

By undefining wcwidth, which at this point is defined to GNULib's prefixed version, rpl_wcwidth, we force the use of the system implementation. CPU goes down from ~80-90% to 0.5%.

If the user doesn't have wcwidth in the system library, they don't have an alternative, but most systems should have it.

Apply this patch to clean 2.9.5,

(file #43970)

easyaspi314 <easyaspi314>
Mon 30 Oct 2017 08:50:28 PM UTC, comment #12: 

Okay.  I suppose that something like it would only be acceptable if it's part of a larger attempt at caching softwrap data, but that would of course take much more time and effort to implement.

David Lawrence Ramsey <dolorous>
Group Member
Mon 30 Oct 2017 08:18:52 PM UTC, comment #11: 

There is some improvement, but it's "too small" -- it's not a difference of day and night, so I don't think it's worth the extra code.

Benno Schulenberg <bens>
Group administrator
Sun 29 Oct 2017 07:25:37 PM UTC, comment #10: 

Since it's been awhile, I've attached a resync of the second patch against git 3f27c31.

(file #42293)

David Lawrence Ramsey <dolorous>
Group Member
Thu 20 Jul 2017 10:44:25 AM UTC, comment #9: 

For now, I've just applied the second patch (after updating it for the unchanged line), as it's obviously correct.

With the second patch, I can notice some improvement in the horizontal and vertical cursor movement when being far into a softwrapped line, but... I'll refrain for now.

Benno Schulenberg <bens>
Group administrator
Thu 20 Jul 2017 03:24:32 AM UTC, comment #8: 

Forget that last patch; it doesn't build in tiny mode.

The attached new patch does.  Also, there's a second attached patch after it, that properly makes checks for partially-offscreen lines in edit_scroll() only get run in softwrap mode (since that's the only way lines can be partially offscreen).

(file #41258, file #41259)

David Lawrence Ramsey <dolorous>
Group Member
Thu 20 Jul 2017 01:44:19 AM UTC, comment #7: 

How about the attached patch, against git 0e4cbd4?  It saves the number of softwrapped chunks before firstcolumn in the openfilestruct, and only changes it when firstcolumn changes, thus eliminating the need to calculate it on the fly every single time.

(file #41257)

David Lawrence Ramsey <dolorous>
Group Member
Wed 19 Jul 2017 09:40:38 AM UTC, comment #6: 

Up to five thousand columns, performance is acceptable.  But when the cursor is around column ten thousand... even every move of the cursor lags, also a simple <Left> or <Right> -- which don't change chunk and should therefore be instantaneous.

The slowness for <Left> and <Right> is in place_the_cursor(), I think: it first runs through all the chunks up to firstcolumn, and then goes through all the chunks again up to xpt.  Perhaps there is a way to optimize this a bit when edittop == current?  (Or maybe all the movement routines, when in softwrap mode, should keep track of row and col themselves?)

Anyway, the check for "line != NULL" in that routine should be superfluous -- if it were to reach NULL, something is wrong and it should call "Mayday", or just crash.

Benno Schulenberg <bens>
Group administrator
Tue 18 Jul 2017 12:01:41 AM UTC, comment #5: 

Besides, the new softwrap treats two-column control characters and two-column Unicode characters in the same fashion, in terms of display and movement.  Given that, in terms of softwrap navigation, there's no difference between a binary file (a bunch of two-column control characters, plus irregular spacing and tabs) and a text file written in, say, two-column Chinese characters, with the same irregular spacing and tabs, plus a mixture of Unix, DOS, and Mac newlines.

So it seems to me that the idea that "nano's behavior doesn't matter so much under such circumstances" is a dangerous road to go down, as similar circumstances can occur with the right (admittedly unusual) text files.

David Lawrence Ramsey <dolorous>
Group Member
Mon 17 Jul 2017 07:55:09 PM UTC, comment #4: 

Define "if the scrolling behaves a little weird there".  nano isn't meant to be a binary editor, no, but it should still function properly if the file contains binary garbage.

David Lawrence Ramsey <dolorous>
Group Member
Mon 17 Jul 2017 06:52:06 PM UTC, comment #3: 

Hmmm...  Working backward (without --atblanks) might put a leftedge one (single-width) character too far back, but... if my powers of imagination are right, this one-off placement cannot accumulate, the misplacement cannot get bigger than one (single-width) character off.

It will look silly, of course, when finally reaching the real start of the line, to see a bunch of characters jumping from the start of rows to the end of the preceding row, but... how often would it happen?  Mostly files will consist of either single-width or double-width characters, and then there will be no ambiguity: you can walk backward perfectly and find all the correct leftedges.  Binary files are a mess, of course, and one shouldn't edit them anyway, so it doesn't matter if the scrolling behaves a little weird there.

So... I would be curious to see how it would work in practice.

Benno Schulenberg <bens>
Group administrator
Mon 17 Jul 2017 03:56:19 PM UTC, comment #2: 

Actually, after some checking, edit_scroll() does have to redraw the entire line, but that's because it uses update_softwrapped_line(), which does that.  That's useful if the text has changed, but in this case, we've only moved, so it hasn't.

David Lawrence Ramsey <dolorous>
Group Member
Mon 17 Jul 2017 03:47:31 PM UTC, comment #1: 

Redrawing the screen already tells ncurses to move the text one row down and redraw only the top row; that's what edit_scroll() does, and M+- indirectly calls it.

Moving back one chunk is the slow part, because with softwrapped chunks' now having variable length, nano has to scan the entire line to determine where the leftedges are, since their positions depend on the actual text of the line (although less so when not using --atblanks).

Moving back character by character as you suggest won't fix this, since nano would still have to know in advance where all the leftedges are in order to know when to stop moving.  Some way of caching the leftedges would fix it, though.

David Lawrence Ramsey <dolorous>
Group Member
Mon 17 Jul 2017 11:55:56 AM UTC, original submission:  

Open some file that contains a line of enormous length.  Find that line and press <End> <End> on it, so we're actually at the very end of that line.  Then type M+- (that is: order nano to move the whole text one row down).  This takes something like two seconds.

Where is the slowness?  In the actual moving back one chunk?  Or in the redrawing of the screen?  (Does the entire screen get redrawn?  This should not be necessary: it can just tell ncurses to move all the text one row down and then redraw only the top row, no?)

If the slowness is in moving back one chunk, maybe that routine can be modified to actually move back character by character by at most editwincols from the x position that corresponds to firstcolumn?  Maybe that x position needs to be stored too so that it doesn't need to be recomputed?

Benno Schulenberg <bens>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #43970:  no_gnulib_wcwidth.patch added by easyaspi314 (602B - application/octet-stream - Patch overrides gnulib's wcwidth implementation for the faster system version.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by easyaspi314 (Updated the item)
  • -email is unavailable- added by dolorous (Posted a comment)
  • -email is unavailable- added by bens (Submitted the item)
  • -email is unavailable- added by bens
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-12-01 bens StatusNone Wont Fix
        Open/ClosedOpen Closed
    2018-07-13 bens Severity3 - Normal 2 - Minor
        StatusReady For Test None
        Assigned tobens None
    2018-06-04 bens StatusNone Ready For Test
        Assigned toNone bens
    2018-04-18 easyaspi314 Attached File- Added no_gnulib_wcwidth.patch, #43970
    2017-10-29 dolorous Attached File- Added 0001-softwrap-save-the-number-of-softwrapped-chunks-befor.patch, #42293
    2017-07-20 dolorous Attached File- Added 0001-softwrap-save-the-number-of-softwrapped-chunks-befor.patch, #41258
        Attached File- Added 0002-softwrap-only-compensate-for-partially-offscreen-lin.patch, #41259
    2017-07-20 dolorous Attached File- Added 0001-softwrap-save-the-number-of-softwrapped-chunks-befor.patch, #41257
    2017-07-19 bens Summaryscrolling up one row on a huge softwrapped line takes too long any cursor movement on a huge softwrapped line is slow
    2017-07-17 bens Carbon-Copy- Added dolorous

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code