bugGNU nano - Bugs: bug #54928, [Task] a syntax file should be...

 
 

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

bug #54928: [Task] a syntax file should be fully read only when the syntax gets actually used

Submitter:  Benno Schulenberg <bens>
Submitted:  Wed 31 Oct 2018 06:40:36 PM UTC
   
 
Severity:  3 - Normal Status:  Fixed
Assigned to:  bens Open/Closed:  Closed

Jump to the original submission

Wed 19 Jun 2019 06:28:15 AM UTC, comment #17: 

Released in nano-4.3.

Benno Schulenberg <bens>
Group administrator
Mon 03 Jun 2019 02:13:58 PM UTC, comment #16: 

(On my old laptop, this change made startup /feel/ slower.  The real startup time has gone down from around 120 ms to around 90 ms (only a dozen syntaxes are included), but instead of there being a slight pause and then two quick screen changes (that feel as one), the screen changes immediately (or so it feels) when <Enter> is pressed, and then there is a slight pause before the file contents are shown.  Indeed, the "pause" between "Reading..." and the contents being shown has become longer, because previously all regexes were compiled while the rcfiles were being parsed, but now the regexes for the active syntax get parsed only when the text file has already been read in.  If the machine is fast enough and the syntax not too compicated, startup will be close enough to immediate that this extended delay between the two screen changes won't be noticed, but on slower machines...  So, in git, I've tried to trim and reshuffle things a bit, to squeeze a few more nanoseconds out of the startup time.)

Benno Schulenberg <bens>
Group administrator
Mon 20 May 2019 08:39:54 AM UTC, comment #15: 

Fixed in git, commit cba9d8d0.  Thanks for the patches.

Without the patches, reading the standard set of syntax files took around 58 ms on my machine.  With the patches, it now takes just 6 ms.  This reduction is noticeable: from the tiniest delay between pressing <Enter> and nano's screen appearing, it is now virtually immediate (when everything is cached).  Nice.

Benno Schulenberg <bens>
Group administrator
Mon 20 May 2019 12:16:53 AM UTC, comment #14: 

Looks okay, but the expression value for MAXSIZE should probably be wrapped in parentheses to make it easier to read and avoid any issues if the code that uses it ever changes.

Brand Huntsman <brand>
Sun 19 May 2019 11:08:12 AM UTC, comment #13: 

Attached are the two patches, with tweaked commit messages, some normalized whitespace, and a few reshufflings.  Some things I will rename later.  Okay to apply in this state?

(file #46928, file #46929)

Benno Schulenberg <bens>
Group administrator
Tue 14 May 2019 12:51:30 AM UTC, comment #12: 

First patch no longer displays errors until finish() or die() is called. Both patches have more descriptive commit messages.

(file #46902, file #46903)

Brand Huntsman <brand>
Mon 06 May 2019 07:41:15 AM UTC, comment #11: 

I can find only two exits that might want to display the error messages, the exit(1) in nano.c when initscr() == NULL (which doesn't have an error message itself, which is not right) and the check_vitals_mapped() in rcfile.c.  But for both of these it might be actually better to not display any other error messages, as they would distract from the essential: initscr() failed / a required key binding is missing.  So... I would prefer the simpler system: print all messages only upon finish() or die().

Benno Schulenberg <bens>
Group administrator
Sun 05 May 2019 09:42:55 PM UTC, comment #10: 

There are exits that would need to display the errors, and exits added later on might not display the errors. The lines of code saved would only be moved to the more complicated exits, which might be aborting due to an undisplayed error.

I'll redo the commit messages in a day or two.

Brand Huntsman <brand>
Sun 05 May 2019 03:57:03 PM UTC, comment #9: 

Why bother with pausing rcfile errors?  Why not always wait with printing all stored messages until exit time?

The patches need commit messages that explain roughly what they do.

Benno Schulenberg <bens>
Group administrator
Sun 28 Apr 2019 09:46:25 AM UTC, comment #8: 

No, but these new patches compile on current git.

(file #46843, file #46844)

Brand Huntsman <brand>
Sun 28 Apr 2019 08:55:16 AM UTC, comment #7: 

I'm going to look into applying this.  Have you meanwhile maybe made further improvements to the patch set?

Benno Schulenberg <bens>
Group administrator
Fri 15 Mar 2019 09:57:59 AM UTC, comment #6: 

New patches. The second patch now sets colorpairs for syntaxes defined in nanorc (oops). The first patch now checks on_a_vt when resuming rcfile errors so they display on exit after reading from stdin on a linux console (https://savannah.gnu.org/bugs/?55912).

From commit 19a833c94e2066750d1ca3cca7ec538ed40b83ed:
"The first probe for a Linux VT is kept in place for now, so that nano can stop when there are errors in an rcfile while running on a VT. (This stopping will not work when data is read from standard input, of course, but that is a smaller fish.)"

With the first patch, that first probe could be removed if nothing else needs it.

(file #46544, file #46545)

Brand Huntsman <brand>
Sun 24 Feb 2019 04:11:30 AM UTC, comment #5: 

https://raw.githubusercontent.com/brndnmtthws/conky/master/extras/nano/conky.nanorc

Conky's syntax file is the main cause of memory bloat, a 6k file using 1.8meg of memory. The patch only reduces memory by 100k with nano's default syntaxes, and a C file loaded. Other file types could see a reduction up to 600k, or 1.2meg in your case (64bit?).

But it really helps out when including user syntax files and those from programs like conky. Depending on the file type, it is up to a 3.4meg reduction with my syntax files and conky.

Brand Huntsman <brand>
Fri 22 Feb 2019 12:32:39 PM UTC, comment #4: 

Well, then the commit message of the first patch should have explained why the change is needed: that the second patch changes things so that a syntax file is fully parsed only when the syntax gets actually used, which means that any errors that it might contain would spill onto the status bar and the help lines and (if there are more) even into the edit window, after which they get mostly overwritten by the contents of the buffer, by the error message on the status bar, and by the help lines; and thus they are unreadable, and when nano exits they are gone.

(In the below, I'm still using only the second patch -- the existing syntax files don't contain any errors, so there is no problem.)

Without the patch:
VmSize:    19028 kB
VmRSS:     4644 kB
VmData:     1884 kB

With the patch:
VmSize:    18796 kB
VmRSS:     4300 kB
VmData:     1652 kB

(This reduction of 344 kB is more than I saw yesterday; then it was less than a hundred kilobytes in most cases.)

With patch and using --ignore:
VmSize:    18004 kB
VmRSS:     3372 kB
VmData:      860 kB

(All this is for running 'src/nano src/nano.c' with all predefined syntaxes included and two 'extendsyntax c ...' commands in ~/.nanorc.)

Benno Schulenberg <bens>
Group administrator
Fri 22 Feb 2019 01:35:15 AM UTC, comment #3: 

The second patch is useless without the first patch. Syntaxes are parsed after ncurses is active and any errors would overwrite the buffer. Fixing the linux console issue was merely a bonus of a patch needed before this feature could be implemented.

Are you looking at the RSS value? What is the memory difference when nanorc is ignored? I have 54 active syntax files with a total of 682 color commands, and ignoring nanorc decreases memory by ~3meg.

Brand Huntsman <brand>
Thu 21 Feb 2019 07:57:38 PM UTC, comment #2: 

The first patch does not have anything to do with this issue.  Please put it somewhere else.

I've tried the second patch (after adjusting it to apply to master).  It cuts load time roughly in half (when including all of nano's syntaxes), which is good.  But I'm not seeing a significant change in memory usage, just some handfuls of kilobytes, less than ten percent.  I'm using "grep Vm /proc/$(pidof nano)/status" to look at memory stats.

Benno Schulenberg <bens>
Group administrator
Thu 21 Feb 2019 01:23:40 PM UTC, comment #1: 

First patch queues up rcfile errors that occur after window_init(), and sends them to terminal in finish() or die(). This allows nano to startup on linux console even with rcfile errors.

Second patch only reads syntax/header/magic commands from included syntax files. Syntaxes defined in nanorc are fully read and extendsyntax stores the command string in each unloaded syntax. When a file uses a syntax, it is read, parsed and extendsyntax commands applied to it. Syntaxes are not unloaded when a buffer using them is closed.

When opening nano.c, RSS decreased from 6024 to 3560 and warm load time decreased from 0.046s to 0.017s.

(file #46321, file #46322)

Brand Huntsman <brand>
Wed 31 Oct 2018 06:40:36 PM UTC, original submission:  

When nano starts up, it now reads all syntax files that are included and compiles all the regexes contained in them.  But in most cases, only one of these syntaxes gets "applied" (used).  This means that the color regexes for all other syntaxes have been read and compiled for nothing -- a waste of time.

If, at startup, nano would read of each syntax only the 'syntax' line, the 'header' line, and the 'magic' line, and stop reading as soon as some other than those three commands is seen, then that saves a lot of time.  When a syntax is "recognized" and should be applied, then the rest of this syntax file is read and the color regexes and other commands are evaluated.

Currently, there are 45 syntaxes that have 42 file regexes, with 10 headerline regexes, and 20 magic regexes.  In total that are 72 regexes.  Further, there are 563 color and icolor regexes.  So, instead of compiling 72+553 = 635 regexes at startup, now only 72 are compiled, plus the ten or twenty for the syntax to be applied.  That is a reduction of roughly 85 percent.

Benno Schulenberg <bens>
Group administrator

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by brand (Updated the item)
  • -email is unavailable- added by bens (Submitted the item)
  •  

    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 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-06-19 bens Open/ClosedOpen Closed
    2019-06-18 bens Summarya syntax file should be fully read only when the syntax gets actually used [Task] a syntax file should be fully read only when the syntax gets actually used
    2019-05-20 bens StatusIn Progress Fixed
    2019-05-19 bens Attached File- Added 0001-rcfile-store-errors-and-display-them-when-nano-termi.patch, #46928
        Attached File- Added 0002-rcfile-fully-parse-a-syntax-file-only-when-needed.patch, #46929
    2019-05-14 brand Attached File- Added 0001-rcfile-store-errors-and-display-when-nano-terminates.patch, #46902
        Attached File- Added 0002-rcfile-load-syntax-files-only-when-needed.patch, #46903
    2019-04-28 brand Attached File- Added nanorc-0001-store-rcfile-errors-and-display-when-nano-terminates.patch, #46843
        Attached File- Added nanorc-0002-load-syntax-files-only-when-needed.patch, #46844
    2019-04-28 bens Assigned toNone bens
    2019-03-15 brand Attached File- Added nanorc-0001-store-rcfile-errors-and-display-when-nano-terminates.patch, #46544
        Attached File- Added nanorc-0002-load-syntax-files-only-when-needed.patch, #46545
    2019-02-21 bens StatusNone In Progress
    2019-02-21 brand Attached File- Added nanorc-0001-store-rcfile-errors-and-display-when-nano-terminates.patch, #46321
        Attached File- Added nanorc-0002-load-syntax-files-only-when-needed.patch, #46322

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code