bugGNU RCS - Bugs: bug #40200, performance regression in newer...

 
 

bug #40200: performance regression in newer versions of RCS

Submitter:  Andrew J. Schorr <ajschorr>
Submitted:  Mon 07 Oct 2013 09:48:32 PM UTC
   
 
Category:  None Severity:  2 - Minor
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  ttn
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 29 May 2018 01:45:11 PM UTC, comment #12: 

RCS 5.9.2 reverts to RCS 5.7 behavior (using mmap(2), if possible) for the case when env var ‘RCS_MEM_LIMIT’ is unset.

Changing status to Fixed, and closing this bug.

Thien-Thi Nguyen <ttn>
Group administrator
Tue 22 Oct 2013 09:04:30 AM UTC, comment #11: 

Thanks for testing.  I will change the status now to Postponed, as the rest of the performance difference between 5.7 and current code is mostly due to architectural changes (i believe), and the search for any implmentation-specific performance problems will not be worth the trouble.

That said, if anyone (reading this) wants to performance profile 5.7 vs current code, then this is the right thread to post findings.  Maybe i'm wrong (happens often enough) and we can indeed fix some things before the checksums are in place.

Thien-Thi Nguyen <ttn>
Group administrator
Mon 21 Oct 2013 06:35:28 PM UTC, comment #10: 

I applied that patch to 5.9.1.  With the patch, it takes
the same amount of time as in my previous test with a large value for RCS_MEM_LIMIT.


Andrew J. Schorr <ajschorr>
Sun 20 Oct 2013 09:39:28 AM UTC, comment #9: 

I understand.  Please find attached a patch, a subset of the one i just pushed:


(The full change also updates the documentation.)  Could you please try it?  I'm changing the status to Ready for Test, as well.


(file #29423)

Thien-Thi Nguyen <ttn>
Group administrator
Fri 18 Oct 2013 04:46:28 PM UTC, comment #8: 

I can give it a try.  My actual usage case is a script
that loops over each saved version of the file.  So if
5.9.1 is 2.5 times as slow as 5.7 (as with the
unlimited RCS_MEM_LIMIT), I still may not want to use it.

Andrew J. Schorr <ajschorr>
Fri 18 Oct 2013 04:34:43 PM UTC, comment #7: 

Yes, 5.7 behavior is/was consistent w/ "unlimited" RCS_MEM_LIMIT.  Thanks for the feedback.  I will install that change shortly.  Would you be willing to try a patch against 5.9.1?

Thien-Thi Nguyen <ttn>
Group administrator
Fri 18 Oct 2013 03:58:28 PM UTC, comment #6: 

Was the behavior of version 5.7 consistent with setting RCS_MEM_LIMIT to unlimited?  If yes, then that sounds good.
If you change the arbitrary cutoff from 256K to 100M, it still
runs the risk of a severe performance regression when people
go over the limit.  It never even occurred to me to check
the man page.  I have been using RCS for many years (decades?),
so why would I suddenly wonder if I were using it incorrectly?

IMHO, these are supposed to be simple, fast, basic, and reliable
tools.  The user shouldn't have to read the man page to
figure out how to configure it to work with acceptable performance.
If I want complexity, I'll use git!

Andrew J. Schorr <ajschorr>
Fri 18 Oct 2013 03:54:45 PM UTC, comment #5: 

Good.  Given this (expected) result, i think the nature of the bug starts to appear as "insufficient documentation" and/or "bad (out of touch w/ modern systems) defaults", both of which strike me as less than grave in severity.  So that explains the drop in severity to Minor.

Still, aside from making ‘RCS_MEM_LIMIT’ more prominant in the docs (which i suppose few read, anyway), there is something we can do in the bug/bugfix frame of mind: find a better default value than the archaic 256.  How does 100 megabytes sound?  Another idea is to change the interpretation of unspecified ‘RCS_MEM_LIMIT’ from a default value to "unlimited".  Maybe that would be better?

Thien-Thi Nguyen <ttn>
Group administrator
Fri 18 Oct 2013 02:01:43 PM UTC, comment #4: 

Yes, that helps a lot.  Here is the performance of 5.7:

bash-4.2$ co -V
RCS version 5.7
bash-4.2$ time co -p1.1 Index | wc
RCS/Index,v  -->  standard output
revision 1.1
  13212  183100  971257

real    0m0.299s
user    0m0.285s
sys     0m0.026s

Here is 5.9.1 without setting RCS_MEM_LIMIT:

bash-4.2$ co --version
co (GNU RCS) 5.9.1
Copyright (C) 2010-2013 Thien-Thi Nguyen
Copyright (C) 1990-1995 Paul Eggert
Copyright (C) 1982,1988,1989 Walter F. Tichy, Purdue CS
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
bash-4.2$ time co -p1.1 Index | wc
RCS/Index,v  -->  standard output
revision 1.1
  13212  183100  971257

real    1m42.994s
user    1m37.399s
sys     0m4.564s

And with RCS_MEM_LIMIT:

bash-4.2$ ls -l RCS/Index,v
-r--r--r-- 1 usdb ead 54975095 Oct 17 18:31 RCS/Index,v
bash-4.2$ export RCS_MEM_LIMIT=54272
bash-4.2$ time co -p1.1 Index | wc
RCS/Index,v  -->  standard output
revision 1.1
  13212  183100  971257

real    0m0.773s
user    0m0.759s
sys     0m0.027s

It is still slower than 5.7, but it's not horrendous.

Andrew J. Schorr <ajschorr>
Fri 18 Oct 2013 10:46:31 AM UTC, comment #3: 

BTW, RCS consults env var ‘RCS_MEM_LIMIT’ to determine whether or not to fall back to (slow) stdio mode (which uses temporary files) from the default in-memory (mmap et al) mode.  The default value is "256" (kilobytes), which is probably too low these days on most workstations.

Please try that command with ‘RCS_MEM_LIMIT=N’ in the environment, where N is the size of the comma-v file, rounded up to the next megabyte, but expressed in kilobytes.  For example, if the comma-v file is 1010456 bytes, then the rounded-up value is 1048576 (1 megabyte) and N will be 1024.

Does that help?

Thien-Thi Nguyen <ttn>
Group administrator
Fri 18 Oct 2013 09:22:20 AM UTC, comment #2: 

Another factor, probably more important in this (-p) case, is that 5.7 co managed do its work w/o temporary files, whereas current co creates a temporary file for each revision.  This can be seen w/ strace(8), for example.  Well, that's clearly bogus and should be fixed!

So, i'm changing the status to In Progress and will followup later w/ more analysis and a fix.

Thien-Thi Nguyen <ttn>
Group administrator
Tue 15 Oct 2013 07:44:37 AM UTC, comment #1: 

Prior to 5.8, RCS was fast and loose with the trailing part of the comma-v file, leading in some cases to long-standing undetected corruption.  As of 5.8, each access involves an integrity check, which involves scanning the entire file and results in the slowdown you see.  So yes, i can confirm that the performance regression is known.

The way to achieve both integrity and performance is to use some kind of checksum so that the scan need not be as detailed.  That feature is still in the design stage, however.

Thien-Thi Nguyen <ttn>
Group administrator
Mon 07 Oct 2013 09:48:32 PM UTC, original submission:  

Hi,

On a newly installed Fedora 19 system running version 5.9 of RCS, I noticed that it takes an incredibly long time to run "co -p1.1 <filename>" on a file that has 3585 revisions in its history.  It takes over 1 minute.  By comparison, on Fedora 16 with version 5.7 of RCS, it takes less than 1 second.  I built 5.7 on the new O/S to confirm that it's not an O/S problem, and there are no performance problems with version 5.7 running on Fedora 19.  I started to run "git bisect" to find the problem, but I ran into trouble, since intermediate versions would not build.  So I gave up and installed 5.7 on my Fedora 19 systems.

N.B. I found the same performance problems in versions 5.8.1 and 5.8.2.

I opened this Fedora bug as well.
https://bugzilla.redhat.com/show_bug.cgi?id=1016308

Regards,
Andy

Andrew J. Schorr <ajschorr>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #29423:  unlim.diff added by ttn (3KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by ttn (Posted a comment)
  • -email is unavailable- added by ajschorr (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.

    Only logged-in users can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-05-29 ttn StatusPostponed Fixed
        Open/ClosedOpen Closed
    2013-10-22 ttn StatusReady For Test Postponed
    2013-10-20 ttn StatusIn Progress Ready For Test
    2013-10-20 ttn Attached File- Added unlim.diff, #29423
    2013-10-18 ttn Severity3 - Normal 2 - Minor
    2013-10-18 ttn StatusConfirmed In Progress
    2013-10-15 ttn StatusNone Confirmed
        Assigned toNone ttn

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code