patchGNU nano - Patches: patch #9641, use unlocked reading for faster...

 
 

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

patch #9641: use unlocked reading for faster reads

Submitter:  easyaspi314 <easyaspi314>
Submitted:  Sat 26 May 2018 03:56:54 AM UTC
   
 
Priority:  5 - Normal Status:  Done
Privacy:  Public Assigned to:  bens
Open/Closed:  Closed Release:  None

Jump to the original submission

Sun 23 Sep 2018 06:34:01 AM UTC, comment #27: 

Was released in 3.0.

Benno Schulenberg <bens>
Group administrator
Thu 12 Jul 2018 10:52:06 AM UTC, comment #26: 

Thanks for confirming.  Pushed to master, commit b2ff5746.

Benno Schulenberg <bens>
Group administrator
Thu 12 Jul 2018 12:58:55 AM UTC, comment #25: 

Oh, sorry for the inactivity.

It looks fine, go ahead. The getc_unlocked alone is enough to dramatically speed up things on BSD/Android.

easyaspi314 <easyaspi314>
Tue 10 Jul 2018 12:39:37 PM UTC, comment #24: 

Devin, I propose to apply the attached version of the patch.  Okay with you?

(file #44534)

Benno Schulenberg <bens>
Group administrator
Mon 09 Jul 2018 09:44:36 PM UTC, comment #23: 

file #44520 is about 2-3ms faster than file #44334. My patch (file #44288) was about 10ms slower than this patch (file #44334) but a kernel upgrade has made my patch 35ms slower on cached reads and 50ms slower on cold reads.

Brand Huntsman <brand>
Sun 08 Jul 2018 08:17:14 AM UTC, comment #22: 

I have reduced your last patch to just the getc_unlocked() call and the separate locking and unlocking-- see attached.  This by itself already brings the loading time down from 1.2 to .75 seconds on my system.  How much reduction do you guys get?

(It seems that the default buffer size is the optimum one -- or so close to it that changing it is not worth the trouble.)

(file #44520)

Benno Schulenberg <bens>
Group administrator
Sat 07 Jul 2018 08:21:11 PM UTC, comment #21: 

On my system, file #44288 is consistently .01 seconds faster than file #44334.  But that seems to be because of the 8K buffer used in the latter.  When I make that a 4K buffer, the fastest time of file #44334 equals that of file #44288, but the variation is much bigger -- on average file #44288 still is faster.

In file #44334 I hate the abundance of setvbuf() calls.  Also, the LOCKBUFSIZE constant should not have been renamed -- a separate constant should have been used: the two have nothing to do with each other: one should be able to change the size of each separately.

Benno Schulenberg <bens>
Group administrator
Mon 11 Jun 2018 02:28:58 AM UTC, comment #20: 

Oops. I saw the charalloc in do_lockfile and missed the fact that it was modifying existing code, which does have a free, just not in the patch.

Brand Huntsman <brand>
Sun 10 Jun 2018 01:17:28 PM UTC, comment #19: 

You mean the one in setvbuf?

That one is auto-freed as long as you pass NULL to the second argument.

If you are talking about my previous patch, I allocated it on the stack. No malloc required.

easyaspi314 <easyaspi314>
Sun 10 Jun 2018 02:47:16 AM UTC, comment #18: 

The patch improved performance in all cases on my system, even if the cold read difference was insignificant. I'll probably never see the 100% improvement I saw loading the cached 29meg file, but I'd like to know it was there if I do need it, and I love all insignificant performance increases (they add up). If no one else sees a performance decrease then I say the patch should be added.

The only problem I see is the buffer it allocates and never frees, unless I missed the free. Is it important to keep that buffer around after the file has been loaded?

Brand Huntsman <brand>
Sun 10 Jun 2018 01:25:05 AM UTC, comment #17: 

Oh. Apparently flockfile is nooped if you don't enable pthreads on glibc. You might see this effect if you use -pthread.

Never mind.

Either way, FreeBSD and Bionic get huge benefits here, on both a 5400 RPM and flash memory.

easyaspi314 <easyaspi314>
Sun 10 Jun 2018 01:13:50 AM UTC, comment #16: 

1.635s cold read w/ slow_getc
1.418s cold read
0.664s cached read w/ slow_getc
0.585s cached read

Brand Huntsman <brand>
Sun 10 Jun 2018 12:51:31 AM UTC, comment #15: 

Well, since you have glibc, to see what the difference is in the call on BSD/Android, try replacing the original getc version to use this:


int slow_getc(FILE *f)
{
    int ret;

    flockfile(f);
    ret = getc_unlocked(f);
    funlockfile(f);
    return ret;
}


This should show how much of an improvement these libcs get.

Tell me what your performance difference is.

Also, if you find your 7200 RPM to be too slow, I'd happily trade you my 5400 RPM. :P

easyaspi314 <easyaspi314>
Sat 09 Jun 2018 08:07:58 PM UTC, comment #14: 

This removes a single file (million) from the cache, if you don't want to empty your entire cache.

dd of=million oflag=nocache conv=notrunc,fdatasync count=0

And this can be used to verify it works. The first run is from disk and second run is from cache.

time cat million >/dev/null
time cat million >/dev/null
 
2.392s git without cc2b19c8
1.418s git with cc2b19c8
1.411s git with cc2b19c8 and 4k fread patch
1.408s git with cc2b19c8 and this unlocked getc patch

There is almost no improvement for cold reads from a slow 7200rpm hdd. Reads from cache, other libcs and maybe a fast ssd will see a bigger improvement.

Brand Huntsman <brand>
Sat 09 Jun 2018 05:29:22 PM UTC, comment #13: 

By the way, the approximate equivalent of sudo purge on Linux and Android is

sync; echo 3 > /proc/sys/vm/drop_caches

On my Android, I get 2 seconds on mine vs 3 seconds on the other patch on a cold read.

The other benefit is that this should improve other file IO as well.

easyaspi314 <easyaspi314>
Sat 09 Jun 2018 06:20:46 AM UTC, comment #12: 

This patch is faster than my patch at https://savannah.gnu.org/bugs/?50406 (tested with glibc)

0.320s - git with cc2b19c8 and this patch (file #44334)

Brand Huntsman <brand>
Fri 08 Jun 2018 04:51:24 PM UTC, comment #11: 

Oops, thought I uploaded this. Shows you how forgetful I am. :(

I made it so that all files will open with a buffer of NANO_BUF_SIZE (formerly LOCKBUFSIZE) if it is larger than BUFSIZ via setvbuf() and I now use the fgetc_unlocked() strategy with a single mutex. No progress bar, as you requested.

This makes things a lot faster while not having to worry about a second buffer.

(file #44334)

easyaspi314 <easyaspi314>
Sun 03 Jun 2018 07:41:14 PM UTC, comment #10: 

Ugh. Just realized I lost my post.

Locked I/O is slow as hell but it is thread-safe.

Unlocked I/O is not thread-safe unless you lock the file manually, but it is much faster, as it doesn't require spamming expensive mutexes.

Changing getc to getc_unlocked with a manual lock on Termux cuts from 12s to 2.9s, and with that cut-a-corner patch, I am down to 2.1s.

So, on Bionic libc, the bottleneck wasn't from constantly reading the disk, it was from the locked IO.

And a Unicode file of a similar size (po/zh_CN.po 755 times) loads in 2.9s as well, so there doesn't seem to be a negative impact on it.

I don't think we need a separate buffer now that we found the actual cause, because technically, getc is (usually) buffered and we don't need to reinvent the wheel.

When I get home, I shall check if the theory applies to macOS (which I bet it is because the getc implementations are almost identical), make a patch (because you saw my luck uploading patches on Android) and I think we can do that instead.

easyaspi314 <easyaspi314>
Sun 03 Jun 2018 06:30:51 PM UTC, comment #9: 

What does file locking mean?  And is it the repetitious locking and unlocking that takes so much time on other things than glibc?  Or is it the being "locked" itself that slows things down?

Anyway...  Can you produce a version of the patch without the progress meter and without the "Processing File" stuff?  That is: reduced to just the intermediate buffer to speed things up for you.  And of course, make the size of the buffer a defined constant, so it is easy to change.  And in a separate patch add some code similar to commit 430d3bad to time quite precisely how much time the reading of a file takes.

Starting nano without first reading the entire file would be cool, but that is a far too complicated change.  Let's concentrate on simply making things run faster.

Benno Schulenberg <bens>
Group administrator
Sun 03 Jun 2018 12:46:51 PM UTC, comment #8: 

Bionic libc:

int
getc(FILE *fp)
{
        int c;

        FLOCKFILE(fp);
        c = __sgetc(fp);
        FUNLOCKFILE(fp);
        return (c);
}


Apple/FreeBSD libc:

int
getc(FILE *fp)
{
        int retval;
        FLOCKFILE(fp);
        ORIENT(fp, -1);
        retval = __sgetc(fp);
        FUNLOCKFILE(fp);
        return (retval);
}


GNU glibc:

int
_IO_getc (FILE *fp)
{
  int result;
  CHECK_FILE (fp, EOF);
  if (!_IO_need_lock (fp))
    return _IO_getc_unlocked (fp);
  _IO_acquire_lock (fp);
  result = _IO_getc_unlocked (fp);
  _IO_release_lock (fp);
  return result;
}


As you can see, glibc will check if it needs to lock IO, while FreeBSD and Bionic libc will always lock IO.

easyaspi314 <easyaspi314>
Sun 03 Jun 2018 12:30:19 PM UTC, comment #7: 

Nice find.

Although, for the lack of speedup, I wonder if it is an implementation issue. Termux uses bionic libc and Clang, and macOS uses Apple/FreeBSD libc with Clang as well.

Bionic libc is confusing with its macro mania, but it seems that getc in glibc will only lock the buffer if necessary, while macOS will always lock. Interesting…

However, the other, more complicated problem, is that nano loads the entire file into RAM.

Out of all the TUI editors available in Termux's apt repo, only nano, pico, ne, and micro do this.

Meanwhile, Vim, neovim, Emacs, Joe, busybox vi and especially Jupp (the MirBSD joe fork) will only partially load the file, which, with the exception of Jupp which uses a whopping 12kB RAM, seem to cut off at 64kB.

And on low memory systems or in a thrashing scenario, we don't have that "nano is out of memory" safety net because macOS and Linux rarely ever return NULL on malloc, it just blocks until an opening appears, hanging forever.

easyaspi314 <easyaspi314>
Sun 03 Jun 2018 10:02:28 AM UTC, comment #6: 

A real speedup can be achieved by calling mblen() only for characters that consist of multiple bytes.  See attached patch.  Result (against master, so without other patches applied):

real 0m2,049s
user 0m1,838s
sys 0m0,188s

This works, because most files with consist of plain ASCII, or at least of mostly ASCII -- at least in Europe and thereabouts.

(file #44285)

Benno Schulenberg <bens>
Group administrator
Sun 03 Jun 2018 09:09:11 AM UTC, comment #5: 

Hmm...  I don't get the amount of speedup that you get.  But then I don't have a purge command so I have to run with a hot cache, and I don't have a hard disk but a flash drive (eMMC).

$ time nano fifty

real 0m5,880s
user 0m5,693s
sys 0m0,176s

$ time src/nano fifty

real 0m5,144s
user 0m4,914s
sys 0m0,208s

(The fifty file is fifty times the ./configure script.)

That's roughly 15 percent faster.  If that could be achieved with a little tweak, perfect, but with a patch that adds forty lines...

Also, reading this file happens within a second, it 's the "processing" that takes four, close to five seconds.  What is this processing?  It's not the precalculating of the color info, because I added another statusbar message there and it completes in the blink of an eye.

Pfff...  It's ingrafting the buffer that takes such a long time.  Why?!  What is it doing there?  ...

Brrr...  It's the += get_totsize() that eats the bulk of the time!  Hmmm...  I can gain some time by streamlining mbstrnlen() a bit, copying the needed parts from parse_mbchar().  Result:

real 0m4,690s
user 0m4,352s
sys 0m0,223s

(That's on top of your patch.)  See attached patch.  Further streamlining is possible by just dropping maxlen and by eliding mbstrnlen() as it is called just once.  Hm... strange, that doesn't bring much extra speed:

real 0m4,635s
user 0m4,315s
sys 0m0,224s


(file #44281, file #44282)

Benno Schulenberg <bens>
Group administrator
Sat 26 May 2018 06:33:27 PM UTC, comment #4: 

Thanks for the patch.  Will look at this later -- am concentrating on the justification stuff these days.

Benno Schulenberg <bens>
Group administrator
Sat 26 May 2018 04:05:08 PM UTC, comment #3: 

This is embarrassing. Please ignore that previous patch. That was for dash. Silly thing changed the directory on me.

This should be the correct one. Sorry.

(file #44220)

easyaspi314 <easyaspi314>
Sat 26 May 2018 04:02:48 PM UTC, comment #2: 

Oops, well, I thought I did. Stupid Android browser. :(

The new patch should be attached now.

(file #44219)

easyaspi314 <easyaspi314>
Sat 26 May 2018 04:00:48 PM UTC, comment #1: 

I forced nano to use an int64_t for the file size so the math doesn't overflow on 32-bit.

easyaspi314 <easyaspi314>
Sat 26 May 2018 03:56:54 AM UTC, original submission:  

Instead of repeatedly reading the file byte by byte, nano will now read in 8KiB
chunks. This makes large file reading significantly faster, with a 49MB file
opening 3 times faster.

Additionally, in a non-tiny build on files 8MiB or larger, nano will show the
percentage of the file read, followed by a message saying that it is processing.

This makes it visually faster, to prevent a visual hang.

The 8MiB limit is when the [ Reading File ] message begins to become visible,
at least on my mediocre 2009 MacBook with a 5400 RPM 1 TB drive.

These times are from doing the following:

  1. sudo purge to purge any cache
  2. starting a QuickTime screen recording on my iTerm2 window
  3. opening a 49M file (nano's configure script 50 times)
  4. buffering a ^X to exit as soon as the file is opened (this should really cancel the read, but whatever).


I took screencasts and uploaded them to Streamable below.

Before: https://streamable.com/9dnts

real    0m22.280s
user    0m7.402s
sys     0m0.520s


After: https://streamable.com/7p1g8

real    0m7.166s
user    0m2.213s
sys     0m0.341s


easyaspi314 <easyaspi314>

 

(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 brand (Posted a comment)
  • -email is unavailable- added by bens (Posted a comment)
  • -email is unavailable- added by easyaspi314 (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 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-09-23 bens Open/ClosedOpen Closed
    2018-07-12 bens StatusIn Progress Done
    2018-07-10 bens Attached File- Added 0001-files-speed-up-reading-by-using-getc_unlocked-instea.patch, #44534
        StatusNone In Progress
        Assigned toNone bens
    2018-07-08 bens Attached File- Added 0001-files-use-getc_unlocked-instead-of-getc-to-speed-up-.patch, #44520
        Summaryuse buffered reading for faster reads use unlocked reading for faster reads
    2018-07-07 bens Summaryuse buffered reading for faster reads, and add a progress meter use buffered reading for faster reads
    2018-06-08 easyaspi314 Attached File- Added 0001-Use-manual-lock-getc_unlocked-use-fixed-size-buffer-.patch, #44334
    2018-06-03 bens Attached File- Added cut-a-corner.patch, #44285
        SummaryUse buffered reading and a progress meter for faster reads use buffered reading for faster reads, and add a progress meter
    2018-06-03 bens Attached File- Added streamline-character-counting.patch, #44281
        Attached File- Added streamline-it-further.patch, #44282
    2018-05-26 easyaspi314 Attached File- Added 0001-Use-buffered-reading-and-a-progress-meter-for-faster2.patch, #44220
    2018-05-26 easyaspi314 Attached File- Added 0001-Fix-some-portability-issues.patch, #44219
    2018-05-26 easyaspi314 Attached File- Added 0001-Use-buffered-reading-and-a-progress-meter-for-faster.patch, #44217

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code