[PATCH] Stop using CAS

Philip Martin <philip@codematters.co.uk> writes:

> Branko Èibej <brane@xbc.nu> writes:
>
>> I didn't notice any performance hits when running the tests -- not
>> that I'd expect any in that context.
>
> On my Linux box using the generic CAS implementation in apr_atomic.c
> on a non-NPTL 2.4 kernel, rough measurements indicate that running
> 'svn status' on a Subversion working copy is about 3% slower.

I don't have a Windows box to test on, but I hacked the Unix client
and it seems to work. Anyone object to the following implementation?

Stop using atomic CAS operations for the ASP.NET hack.

* subversion/libsvn_wc/adm_files.c
  (adm_dir_name): Change type.
  (ADM_DIR_NAME): Remove.
  (svn_wc_set_dir_name): Stop using CAS.
  (svn_wc_is_adm_dir, v_extend_with_adm_name): Stop using ADM_DIR_NAME.

* subversion/include/svn_wc.h
  (svn_wc_set_dir_name): Tweak docstring.

Index: subversion/libsvn_wc/adm_files.c
===================================================================
--- subversion/libsvn_wc/adm_files.c (revision 16327)
+++ subversion/libsvn_wc/adm_files.c (working copy)
@@ -58,18 +58,14 @@
 /* The name that is actually used for the WC admin directory. The
    commonest case where this won't be the default is in Windows
    ASP.NET development environments, which choke on ".svn". */
-static void *volatile adm_dir_name = (void*) default_adm_dir_name;
-/* NOTE: we cast away the const here to avoid GCC warnings. */
+static const char *adm_dir_name = default_adm_dir_name;
 
-/* This is an atomic reader for adm_dir_name. */
-#define ADM_DIR_NAME apr_atomic_casptr (&adm_dir_name, NULL, NULL)
 
-
 svn_boolean_t
 svn_wc_is_adm_dir (const char *name, apr_pool_t *pool)
 {
   (void)pool; /* Silence compiler warnings about unused parameter */
- return (0 == strcmp (name, ADM_DIR_NAME)
+ return (0 == strcmp (name, adm_dir_name)
           || 0 == strcmp (name, default_adm_dir_name));
 }
 
@@ -95,18 +91,8 @@
   for (dir_name = valid_dir_names; *dir_name; ++dir_name)
     if (0 == strcmp (name, *dir_name))
       {
- void *const new_name = (void*) *dir_name;
- while (1)
- {
- void *const old_name = ADM_DIR_NAME;
- if (old_name == apr_atomic_casptr (&adm_dir_name,
- new_name,
- old_name))
- return SVN_NO_ERROR;
-
- /* Another thread won the race to change the name; retry. */
- /* ### Should we put a random sleep here? */
- }
+ adm_dir_name = *dir_name;
+ return SVN_NO_ERROR;
       }
   return svn_error_createf
     (SVN_ERR_BAD_FILENAME, NULL,
@@ -138,7 +124,7 @@
   const char *this;
 
   /* Tack on the administrative subdirectory. */
- path = svn_path_join (path, ADM_DIR_NAME, pool);
+ path = svn_path_join (path, adm_dir_name, pool);
 
   /* If this is a tmp file, name it into the tmp area. */
   if (use_tmp)
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 16327)
+++ subversion/include/svn_wc.h (working copy)
@@ -357,7 +357,7 @@
  * The list of valid names is limited. Currently only ".svn" (the
  * default) and "_svn" are allowed.
  *
- * @note This function changes global (per-process) state and should,
+ * @note This function changes global (per-process) state and must,
  * to maintain consistency, be called in a single-threaded context
  * during the initialization of a Subversion client.
  *

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org