On Thu, Sep 18, 2014 at 3:00 PM, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 11 September 2014 20:28, Stefan Fuhrmann > <stefan.fuhrm...@wandisco.com> wrote: > > On Wed, Sep 10, 2014 at 5:35 PM, Ivan Zhakov <i...@visualsvn.com> wrote: > >> > >> I've accidentally ran Subversion test suite under elevated local > >> administrator account on Windows and got failures in named-atomics > >> tests: > >> [[[ > >> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:520: > >> (apr_err=SVN_ERR_TEST_FAILED) > >> svn_tests: E200006: assertion 'value == 42' failed at > >> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:520 > >> FAIL: named_atomic-test.exe 1: basic r/w access to a single atomic > >> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:581: > >> (apr_err=SVN_ERR_TEST_FAILED) > >> svn_tests: E200006: assertion 'value == 42 * HUGE_VALUE' failed at > >> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:581 > >> FAIL: named_atomic-test.exe 2: atomics must be 64 bits > >> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:643: > >> (apr_err=SVN_ERR_TEST_FAILED) > >> svn_tests: E200006: assertion 'value1 == 46 * HUGE_VALUE' failed at > >> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:643 > >> FAIL: named_atomic-test.exe 3: basic r/w access to multiple atomics > >> ]]] > >> > >> The investigation revealed the following: > >> 1. The named_atomics tests are skipped unless executed under elevated > >> local administrator on Windows. > > > > > > What worries me more is that our Windows build bots don't report > > those tests as "SKIP" - although the common init_test_shm() function > > in named_atomics-test.c will return SVN_ERR_TEST_SKIPPED. > > Are they reported as skipped in your setup when you run the test > > without local admin rights? > > > I'm getting SKIP notifications for named_atomics tests when run them > without local admin rights. It seems that buildbot executes these tests, > but Windows buildbot uses VS2010 which doesn't have intrinsics for > "efficient" atomics, so it uses different code. > Yes, I identified 32 bits with old SDKs as the ones that won't use intrinsics and, therefore, wouldn't use revprop caching at all. [...] >> 2. Remove it since "named atomics" framework is only used for currently > >> disabled revprop caching. > > > > > > ... I'm +1 on getting rid of the SHM code altogether (we are using MMAP > to > > get shared memory). It turned out to be a poor choice as all sorts of > > Windows > > platform specifics make it hard caused us to deviate further and further > > from > > the initial APR-based design. Some of the Windows-specific issues, e.g. > the > > file retry races in the init code, should have been reported before the > > 1.8.0 > > release. > > > >> > >> Personally I don't see reason to spend resources fixing unused code, > >> especially > >> that even code on 'revprop-caching-ng' branch removes it also. Any > >> other opinions? > > > > > > No, I agree. > > > > The revpro-caching-ng branch pretty much exactly removes the SHM > > code. > I think that the revprop-caching-ng branch should not be merged. So > may be worth to remove named-atomics and SHM code from trunk to turn > the trunk back to the normal state (without unused and not working code) > Here is what I will do on /trunk: * Merge the new revprop caching scheme to FSX. * Remove the SHM-dependent bits from FSFS but keep the actual cache invocations (no-ops since there is no cache instance) at least for now. * Remove the named_atomics code. * Update the revprop-caching-ng branch Why do you think the revprop-caching-ng branch should not be merged? -- Stefan^2.
