bugGNU nano - Bugs: bug #51479, opening a binary file with...

 
 

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

bug #51479: opening a binary file with --softwrap can take ages

Submitter:  Benno Schulenberg <bens>
Submitted:  Sun 16 Jul 2017 06:13:17 PM UTC
   
 
Severity:  3 - Normal Status:  Fixed
Assigned to:  bens Open/Closed:  Closed

Jump to the original submission

Sun 23 Jul 2017 04:01:02 PM UTC, comment #19: 

You're right.  So much code has been rewritten recently, I've lost track of a few bits.  Checking current_y in place_the_cursor() before it's changed should work, then.

David Lawrence Ramsey <dolorous>
Group Member
Sat 22 Jul 2017 07:20:23 PM UTC, comment #18: 

The buffers being in an "inconsistent state" during startup should not be a problem, since titlebar() no longer calls place_the_cursor().  The latter is only called (I think) when the buffer contents have already been drawn on the screen, and thus edittop is necessarily correct.

Benno Schulenberg <bens>
Group administrator
Thu 20 Jul 2017 03:35:27 AM UTC, comment #17: 

Thank you.

I'm not certain yet about where to do the current_y check, though.  Doing it in place_the_cursor() before that function recalculates current_y would seem like the obvious place, but the fact that, when first loading a file, the buffers can be in an inconsistent state (as shown in bug #50054) makes me nervous.

David Lawrence Ramsey <dolorous>
Group Member
Wed 19 Jul 2017 08:24:36 AM UTC, comment #16: 

Okay, I've removed the caps (in your name) with a separate commit: ce68f81b.

And with the subsequent commit 0e4cbd4b, I consider this bug fixed.  Or at least reduced to an acceptable state -- other slownesses now stand out more.

For me, a prerelease is just for translators.  I don't think anyone actually tests those things.  Or at least, far too few to be of any use.  Testing will have to happen in an actual release.

Where would you like to catch current_y being set to an out of range value?

Benno Schulenberg <bens>
Group administrator
Tue 18 Jul 2017 09:52:28 PM UTC, comment #15: 

Addendum: If current_y is being set to an out-of-range value somewhere in actual testing, it's better to catch it where it's being set improperly instead of working around it elsewhere.  Compare your commit 9462ba8: removing the null check there helped catch it properly elsewhere.

David Lawrence Ramsey <dolorous>
Group Member
Tue 18 Jul 2017 09:08:18 PM UTC, comment #14: 

Okay.

There's one oddity: the patch removes the cap on how many chunks nano can go forward without removing the cap on how many it can go back, and all uses of go_back_chunks() make sure that the number of chunks is in screen range, so that cap looks unnecessary to me now.  (adjust_viewport() sets it to current_y, but if current_y were out of range, there would be breakage in other places like do_mouse(), and place_the_cursor() always puts it back in range after the screen is redrawn anyway.)  So would you be willing to meld the attached patch with it?

With that, it seems good to me.  Although if stability is that much of a concern, maybe put out a 2.8.6pre2 with it first to get some extra last-minute testing?

(file #41237)

David Lawrence Ramsey <dolorous>
Group Member
Tue 18 Jul 2017 08:10:09 PM UTC, comment #13: 

The improved patch fixes the misplacement of the cursor at startup.  And in fact it's not the cursor that was misplaced, but firstcolumn did not get the appropriate value: zero, when the cursor is near the top of the file.

With "remembered location" I meant: when using positionlog.  But yes, +1,192 or similar would elicit the same "misdisplay".

It can still be slow, but paging up is always faster than without the patch.  Is it good to go in?  Because I'm not entirely that it doesn't affect some use of go_back_chunks() negatively.

Benno Schulenberg <bens>
Group administrator
Tue 18 Jul 2017 07:26:58 PM UTC, comment #12: 

Seems to work, yes.  I finally see what you're doing: it's something like the optimization in the very first version of the softwrap overhaul in that it skips entire lines' worth of chunks, except that yours overshoots backward and then scans forward to make up for it.

It can still be slow if I'm moving through the nano binary (built with CFLAGS=-g3) with it, but that only really happens now in the one line that's over 150000 columns wide(!), which would cause some slowdown regardless.

"(For some reason the patch puts the cursor on the top row at startup when it's remembered location is somewhere in the first half-screen of chunks.)"

What do you mean by remembered location at startup?  Something like "nano +1,192 somefile.txt" placing the cursor wrongly?  If so, I've tried that with your slightly improved patch, and I can't seem to reproduce it.

David Lawrence Ramsey <dolorous>
Group Member
Tue 18 Jul 2017 12:39:05 PM UTC, comment #11: 

Slightly improved patch attached.

(file #41229)

Benno Schulenberg <bens>
Group administrator
Tue 18 Jul 2017 11:42:21 AM UTC, comment #10: 

(For some reason the patch puts the cursor on the top row at startup when it's remembered location is somewhere in the first half-screen of chunks.)

Benno Schulenberg <bens>
Group administrator
Tue 18 Jul 2017 11:22:18 AM UTC, comment #9: 

Of course my approach will be faster.  See attached patch.

(It may require an amendment for the very special case of editwinrows==1, but I don't care about that now.)

(file #41228)

Benno Schulenberg <bens>
Group administrator
Mon 17 Jul 2017 09:59:05 PM UTC, comment #8: 

Even if your patch worked, your approach wouldn't be any faster.  It's the same basic problem as multibyte character parsing: it's designed mostly to go forward.

Recomputing the current chunk's location is necessary for now (until we start caching the leftedges), because, with variable-length chunks, there's no way to determine what the chunk before the current one is without looking at all the previous ones in sequence, since we have no idea how long each chunk is otherwise.

If you don't understand it, let me give an example.  Say these are three hypothetical softwrapped chunks, on 3 lines:

chunk 1
longer chunk 2
longest chunk 3

If the cursor is on chunk 3 and you want to move up a chunk to chunk 2, the sequence, in pseudocode, goes:

Are we on the first leftedge of the line?  No.  (If we were, we'd move back to the previous line and start on the last leftedge of the previous line.)

What leftedge are we starting on? The leftedge of chunk 3.

We need the leftedge of the column before the leftedge we're starting on (in this case, the column before the leftedge of chunk 3, which is the last column of chunk 2).  Start at the beginning of the line and scan.

The first leftedge is the leftedge of chunk 1.  Is this the leftedge we need?  No, so keep scanning.

The second leftedge is the leftedge of chunk 2.  Is this the leftedge we need?  Yes, because the last column of chunk 2 is on this chunk.

Does this make it any clearer?  The current code only scans up to the leftedge before the cursor position.  Your attempt would count all the chunks up to the cursor position, which would require scanning the entire line up to that point, and would be slightly slower than the current code.

David Lawrence Ramsey <dolorous>
Group Member
Mon 17 Jul 2017 08:30:36 PM UTC, comment #7: 

The remaining slowness is in go_back_chunks(), I think.  For every chunk it wants to move backward, it recomputes entirely what the current chunk is, again and again and again.  It shouldn't do that.  It should just determine how many chunks the current line has (until the current position), and then either subtract them from the count, or, if the count is smaller than that, move forward from the start of the current line to "the found number of chunks minus the count".

I've attached an attempt at a patch, but it is all wrong, of course -- all these pointers and leftedges baffle me.

(file #41226)

Benno Schulenberg <bens>
Group administrator
Mon 17 Jul 2017 07:49:36 PM UTC, comment #6: 

The routine originally used go_forward_chunks(), during the review of the original softwrap overhaul, but that required using fsfromline() to determine where to go forward from (since the cutting code used it, and there was no guarantee that the line to go forward from existed in that case).  And you said then that doing that was too slow then on large files because of the fsfromline(), so I changed it to use go_backward_chunks() as you suggested.  It was here, and patch 0003 was the one that make uncutting count softwrapped chunks properly.

http://lists.gnu.org/archive/html/nano-devel/2017-02/msg00014.html

So, since it has to accommodate both uncutting and inserting (possibly large) files, which is it?

David Lawrence Ramsey <dolorous>
Group Member
Mon 17 Jul 2017 07:29:21 PM UTC, comment #5: 

No, that was only a partial fix.  It fixes the case of +1, but not the case of +-1 (although it does reduce it's time to half).

Benno Schulenberg <bens>
Group administrator
Mon 17 Jul 2017 07:21:54 PM UTC, comment #4: 

Okay: it is the call of less_than_a_screenful() in read_file() that is slowing things down so terribly...

First, I think the routine should not use go_back_chunks() because this is inherently slower than going forward.

And second: it is silly to call this routine when reading a file into a fresh buffer.  Whether to turn 'focusing' off is only relevant when inserting/pasting something into an existing file.

So... how to distinguish an insertion from a fresh read?  By the parameter 'undoable'.  Only the first is undoable.

Fixed in git, 2a7c9b7f.

Benno Schulenberg <bens>
Group administrator
Mon 17 Jul 2017 03:37:34 PM UTC, comment #3: 

The point I'm trying to make is that some caching will have to be done to fix this, and the question is where to do it without leading to breakage.

David Lawrence Ramsey <dolorous>
Group Member
Mon 17 Jul 2017 11:57:58 AM UTC, comment #2: 

There has to be a way to mitigate this without doing a full cache of all the leftedges...  For a specific case, see bug #51491.

Benno Schulenberg <bens>
Group administrator
Sun 16 Jul 2017 09:01:21 PM UTC, comment #1: 

The problem will occur with any file that has long enough lines, binary or not.  Since binary files aren't text and aren't contrained by line length the way written text would be, it can show up more often with them.  (Take a decently long text file, delete all its newlines so that it consists of one extremely long line, save it, and then make nano load it.  If it's long enough to consist of, say, ~50k all on one line, nano will take ages to open it too.)

The long-term solution, of course, would be caching softwrap-related information (leftedges, number of chunks) instead of calculating it all on the fly, but that would require another rewrite, since it would necessitate putting hooks everywhere the text gets changed.

David Lawrence Ramsey <dolorous>
Group Member
Sun 16 Jul 2017 06:13:17 PM UTC, original submission:  

It only happens for a few binary files, but it so happened that it happened (for me) for nano itself.  Configure nano with disable-libmagic, so we're sure that doesn't get in the way, and compile.  Then run:

  src/nano --ignore --softwrap  +1 src/nano

It says on the status bar that src/nano is not writable, and then it takes a full nine seconds before finally the contents of the file are displayed.

With other files, like /bin/grep or /bin/tar, nano displays them within half a second, and they are about the same size or bigger.  For me it's only with /bin/ip that it also takes around nine seconds before the contents are shown.

(But there's a bigger problem coming up.)

Benno Schulenberg <bens>
Group administrator

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -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
    2017-07-22 bens Open/ClosedOpen Closed
    2017-07-19 bens StatusIn Progress Fixed
        Assigned toNone bens
    2017-07-18 dolorous Attached File- Added remove-go-back-chunks-cap.patch, #41237
    2017-07-18 bens Attached File- Added 0001-softwrap-when-possible-go-back-a-whole-bunch-of-chun.patch, #41229
    2017-07-18 bens Attached File- Added 0001-softwrap-when-possible-move-back-a-whole-bunch-of-ch.patch, #41228
    2017-07-17 bens Attached File- Added attempt-at-speedup.patch, #41226
        StatusNone In Progress
    2017-07-17 bens StatusFixed None
        Assigned tobens None
    2017-07-17 bens StatusNone Fixed
        Assigned toNone bens
    2017-07-17 bens Severity2 - Minor 3 - Normal
    2017-07-16 bens Carbon-Copy- Added dolorous

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code