GNU 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: |
David Lawrence Ramsey <dolorous> |
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> |
Thu 20 Jul 2017 03:35:27 AM UTC, comment #17: Thank you.
|
David Lawrence Ramsey <dolorous> |
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.
|
Benno Schulenberg <bens> |
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> |
Tue 18 Jul 2017 09:08:18 PM UTC, comment #14: Okay.
|
David Lawrence Ramsey <dolorous> |
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.
|
Benno Schulenberg <bens> |
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.
|
David Lawrence Ramsey <dolorous> |
Tue 18 Jul 2017 12:39:05 PM UTC, comment #11: Slightly improved patch attached. |
Benno Schulenberg <bens> |
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> |
Tue 18 Jul 2017 11:22:18 AM UTC, comment #9: Of course my approach will be faster. See attached patch.
|
Benno Schulenberg <bens> |
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.
|
David Lawrence Ramsey <dolorous> |
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".
|
Benno Schulenberg <bens> |
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.
|
David Lawrence Ramsey <dolorous> |
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> |
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...
|
Benno Schulenberg <bens> |
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> |
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> |
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.)
|
David Lawrence Ramsey <dolorous> |
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:
|
Benno Schulenberg <bens> |
Depends on the following items: None found
Items that depend on this one: None found
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/Closed | Open | Closed | |
2017-07-19 | bens | Status | In Progress | Fixed | |
Assigned to | None | 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 | |
Status | None | In Progress | |||
2017-07-17 | bens | Status | Fixed | None | |
Assigned to | bens | None | |||
2017-07-17 | bens | Status | None | Fixed | |
Assigned to | None | bens | |||
2017-07-17 | bens | Severity | 2 - Minor | 3 - Normal | |
2017-07-16 | bens | Carbon-Copy | - | Added dolorous |
Powered by Savane 3.13-02a9.
Corresponding source code
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.