On Thu, May 15, 2014 at 2:43 PM, Bert Huijben <b...@qqmail.nl> wrote:
> > > > -----Original Message----- > > From: stef...@apache.org [mailto:stef...@apache.org] > > Sent: woensdag 7 mei 2014 15:44 > > To: comm...@subversion.apache.org > > Subject: svn commit: r1593015 - in > > /subversion/trunk/subversion/libsvn_fs_fs: fs.c fs.h > > > > Author: stefan2 > > Date: Wed May 7 13:43:55 2014 > > New Revision: 1593015 > > > > URL: http://svn.apache.org/r1593015 > > Log: > > Follow-up to r1591919: Fix fs tests on Windows and non-threaded platforms > > by always using our svn_mutex__t and thus being able to detect recursive > > locking attempts. > > > > For non-threaded platforms, always using svn_mutex__t adds almost zero > > overhead to the FS file locking that we need to do anyways. On Windows, > > the overhead slightly larger but all the APR functions that svn_mutex__* > > calls have very efficient implementations on Windows. So here, the > relative > > overhead is minimal as well. > > > > OTOH, all our tested platforms will detect incorrect / non-portable lock > > usage. > > Not that even after a week the OpenBSD tests are still failing. > Sorry about that! I've been travelling, had checked only for the FSFS bits to test ok on the build bot and did not recheck later. r1595573 seems to finally fix it (we'll know in about 5h) > And I'm not sure if this is really the right way to fix such a corner case > on Windows. > > Why should we do extra work on Windows which has obvious performance side > effects? > The performance impact on Windows should be less 200ns per lock assuming that APR has been built with intrinsics enabled. Even with 3 times that amount, it means that a full (dump+) load of the ASF repo gets about 10s slower - while taking > 10h total. Windows does check per handle, which is really what we need here. We add > the missing checks for platforms that don't check, but adding a check on > platforms that don't need it really make sense. > So, attempts to take out a lock twice (within the same thread) does not result in deadlocks? Our internal API sure makes it a possibility and the *public* svn_fs_freeze can be used recursively as well. Exposing this function in our bindings makes things worse when they try to debug the problem from their respective environments. I understand that the issues caused by two repos falsely sharing the same FS data instance do not exist under WIndows as the repo lock files are still disjoint. IOW, you are right that the deadlock case is more edgy under Windows than on other platforms. I'll remind you of this example the next time you call the Windows > implementation slow. Our Windows code is slow for many reasons but critical sections are none of them. In our case, they are not contented and take about 10ns to acquire. The about 200ns figure is grossly bolstered up to account for APR internal and calling overhead as well as releasing the lock. Sure, Windows is slow if you first want to make it behave exactly like *nix > and then add another layer for the features missing on *nix. > I want our code to be (at least) as robust under Windows as on other platforms. If you are willing to trade a << 0.1% performance again against a theoretical deadlock, then say so. Or demonstrate that either my estimates or the technical rationale behind the deadlock check are wrong. > Same thing as we used to do for obtaining file status... Read the > directory; and then for every node read the directory again because we > assume every system uses i-nodes to hold other things. > Without looking at the code, that sounds like a fair case for platform specific code. Most of our perceived performance issues on Windows are in the working copy code. All power to you if you feel that platform specific code will work better. > These 'performance optimizations' you have been applying over the last few > weeks are slowed Subversion down by +- 2-5% on my test environment when I > measured the result about a week ago. > I remember seeing you comment something like this on IRC (when I scanned the log) and I took it as a "it's seems slower but I can't see why" statement. That weekend I only committed the 'svn log' error code issue and the stream chunk size increase. Neither of which are immediately plausible as a root cause. Can you identify which of the commits in the [1586922:1586953] range causes the slowdown you are seeing? My numbers for ra_local+fsfs over a wider range of revisions: r1577835 (2049 tests) real 87.656s (STD=0.832s), user+sys 495.611s (STD=4.197s) r1583644 (2053 tests) real 87.554s (STD=1.368s), user+sys 495.191s (STD=8.312s) r1593146 (2064 tests) real 88.444s (STD=0.692s), user+sys 499.128s (STD=4.468s) Averages and standard deviation of 3 runs of our test suite with 'time make check PARALLEL=16 > /dev/null' on a ramdisk on my macbook with a static, fully optimized build. Normalized to the same number of tests, the results are almost perfectly stable. > Whatever runs faster on the combination of your huge system, your specific > compiler, your os and your multi gbit network is not necessarily whatever > all others are using. > True. But testing with a macbook against a HDD based server with RAM < repo size over a 1Gbit network seems like a fair test to me. And *then* testing to server throughput and scalability by simulating a network of clients using stock hardware is certainly reasonable as well. OTOH, using the test suite as a benchmark will yield indirect results at best: once you verified that there is a slowdown and identified the root cause, you still have to evaluate the impact on people that don't work on greek trees all day. > And optimizing for most of those specific scenarios is a task for the OS, > compiler, platform libraries etc. Not necessarily always Subversion itself. If the application allocates >30GB of dynamic memory as it was the case until a few weeks ago, $MagicToolOrComponent will have a hard time preventing a crash. As for the 'svn log', we got bitten by limiting our allocator to 4MB of unused memory while Apache's default is 64MB (?) or so. Hence, svnserve was less than half as fast as httpd. Moreover, we would allocate several 100MB per client during "svn log -g". Both issues have been addressed in Subversion itself now. > (E.g. in the recent cases where code was added to support more memory for > apr compiled in a nonstandard configuration) > Using mmap for allocations *is* the standard configuration on some systems. Simply aborting with OOM without indicating what the actual root cause might be is just bad style. -- Stefan^2.
