Re: [xml] Re: Thread-safety of XSLT contexts with shared dictionaries

[Gary Coady <gary lyranthe org>]

Daniel Veillard wrote:
> On Fri, Jan 21, 2005 at 04:22:18PM +0000, Gary Coady wrote:
>
> > Daniel Veillard wrote:
> >
> > > Yup I knew about the potential problem, but it wasn't clear it would end
> > > up being a serious one. I agree that the most generic solution is to
> > > use mutexes to avoid race in the increment/decrement. I'm not sure however
> > > that a global mutex is the best thing. One mutex per dict would avoid 
> > > any initialization race and avoid a global contention point for all threads
> > > while there really should not be global. It is a trade-off between:
> > >  - cost of creating a mutex
> > > and
> > >  - cost associated in being blocked on a global mutex
> > >
> > > I would expect the cost of creating a mutex to be extremely low on
> > > processors/arch with atomic test-and-set operations (and SMP cache
> > > invalidation), on the other hand the cost of a global mutex rise quickly
> > > precisely for the programs where threading is intensive and where that
> > > mutex is really needed.
> > > Any chance you could make another patch where the mutex is in the 
> > > xmlDict structure and initialized in xmlDictCreate(). Also never put
> > > a patch in the mail content dicrectly, add it as an attachment otherwise
> > > the mail programs destroy it so much that patch(1) refuses to apply it.
> >
> > Good point, here's another patch which works in my test setup.
>
>
>   Applied, seems to give around 2% performances loss in kind of worse
> case tests but it's bearable since it an exact solution to a nasty to debug
> race otherwise.
>
>   before (best of 5 tests)
>
> test64:~/XML/xstc -> time nist-test.py -s -b .
> Ran 5430 of 5430 tests: 500 failed 48 unimplemented
>
> real    0m3.301s
> user    0m2.879s
> sys     0m0.422s
> test64:~/XML/xstc ->
>
>   after (best of 5 tests)
>
> test64:~/XML/xstc -> time nist-test.py -s -b .
> Ran 5430 of 5430 tests: 500 failed 48 unimplemented
>
> real    0m3.358s
> user    0m2.951s
> sys     0m0.407s
> test64:~/XML/xstc ->
>
>   Commited. But it would be interesting to see if the global lock one 
> would lead to less performance losses on non threaded applications, could
> you send again the first patch but as an attachment ?

Daniel, thanks for committing the change.

I'm attaching the first patch. Well, mostly the first patch, with some 
extra code to make sure that a failure to initialize the mutex is 
recognised.

Gary.

diff -cr libxml2-2.6.17/dict.c libxml2-2.6.17-patched/dict.c
*** libxml2-2.6.17/dict.c       2005-01-04 14:49:47.000000000 +0000
--- libxml2-2.6.17-patched/dict.c       2005-01-21 17:45:48.634120145 +0000
***************
*** 70,75 ****
--- 70,119 ----
  };
  
  /*
+  * A mutex for modifying the reference counter for shared
+  * dictionaries.
+  */
+ static xmlRMutexPtr xmlDictMutex = NULL;
+ 
+ /*
+  * Whether the dictionary mutex was initialized.
+  */
+ static int xmlDictInitialized = 0;
+ 
+ /**
+  * xmlInitializeCatalog:
+  *
+  * Do the dictionary mutex initialization.
+  * this function is not thread safe, initialization should
+  * preferably be done once at startup
+  */
+ static int xmlInitializeDict() {
+     if (xmlDictInitialized)
+         return(1);
+ 
+     if ((xmlDictMutex = xmlNewRMutex()) == NULL)
+         return(0);
+ 
+     xmlDictInitialized = 1;
+     return(1);
+ }
+ 
+ /**
+  * xmlCatalogCleanup:
+  *
+  * Free the dictionary mutex.
+  */
+ void
+ xmlDictCleanup(void) {
+     if (!xmlDictInitialized)
+         return;
+ 
+     xmlFreeRMutex(xmlDictMutex);
+ 
+     xmlDictInitialized = 0;
+ }
+ 
+ /*
   * xmlDictAddString:
   * @dict: the dictionnary
   * @name: the name of the userdata
***************
*** 277,283 ****
  xmlDictPtr
  xmlDictCreate(void) {
      xmlDictPtr dict;
!   
      dict = xmlMalloc(sizeof(xmlDict));
      if (dict) {
          dict->ref_counter = 1;
--- 321,331 ----
  xmlDictPtr
  xmlDictCreate(void) {
      xmlDictPtr dict;
! 
!     if (!xmlDictInitialized)
!         if (!xmlInitializeDict())
!             return(NULL);
!  
      dict = xmlMalloc(sizeof(xmlDict));
      if (dict) {
          dict->ref_counter = 1;
***************
*** 328,335 ****
--- 376,389 ----
   */
  int
  xmlDictReference(xmlDictPtr dict) {
+     if (!xmlDictInitialized)
+         if (!xmlInitializeDict())
+             return(-1);
+ 
      if (dict == NULL) return -1;
+     xmlRMutexLock(xmlDictMutex);
      dict->ref_counter++;
+     xmlRMutexUnlock(xmlDictMutex);
      return(0);
  }
  
***************
*** 445,453 ****
      if (dict == NULL)
        return;
  
      /* decrement the counter, it may be shared by a parser and docs */
      dict->ref_counter--;
!     if (dict->ref_counter > 0) return;
  
      if (dict->subdict != NULL) {
          xmlDictFree(dict->subdict);
--- 499,517 ----
      if (dict == NULL)
        return;
  
+     if (!xmlDictInitialized)
+         if (!xmlInitializeDict())
+             return;
+ 
      /* decrement the counter, it may be shared by a parser and docs */
+     xmlRMutexLock(xmlDictMutex);
      dict->ref_counter--;
!     if (dict->ref_counter > 0) {
!         xmlRMutexUnlock(xmlDictMutex);
!         return;
!     }
! 
!     xmlRMutexUnlock(xmlDictMutex);
  
      if (dict->subdict != NULL) {
          xmlDictFree(dict->subdict);
diff -cr libxml2-2.6.17/include/libxml/dict.h libxml2-2.6.17-patched/include/libxml/dict.h
*** libxml2-2.6.17/include/libxml/dict.h        2005-01-04 14:49:49.000000000 +0000
--- libxml2-2.6.17-patched/include/libxml/dict.h        2005-01-21 17:32:47.241573760 +0000
***************
*** 56,61 ****
--- 56,68 ----
                                         const xmlChar *str);
  XMLPUBFUN int XMLCALL                 
                        xmlDictSize     (xmlDictPtr dict);
+ 
+ /*
+  * Cleanup function
+  */
+ XMLPUBFUN void XMLCALL
+                         xmlDictCleanup  (void);
+ 
  #ifdef __cplusplus
  }
  #endif
diff -cr libxml2-2.6.17/parser.c libxml2-2.6.17-patched/parser.c
*** libxml2-2.6.17/parser.c     2005-01-16 18:43:47.000000000 +0000
--- libxml2-2.6.17-patched/parser.c     2005-01-21 17:32:47.343554137 +0000
***************
*** 12158,12163 ****
--- 12158,12164 ----
  #ifdef LIBXML_CATALOG_ENABLED
      xmlCatalogCleanup();
  #endif
+     xmlDictCleanup();
      xmlCleanupInputCallbacks();
  #ifdef LIBXML_OUTPUT_ENABLED
      xmlCleanupOutputCallbacks();