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

[Gary Coady <gary lyranthe org>]

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.

Gary.

*** dict.c.orig 2005-01-21 16:16:28.927395653 +0000
--- dict.c      2005-01-21 16:16:37.918688091 +0000
***************
*** 60,65 ****
--- 60,66 ----
   */
  struct _xmlDict {
      int ref_counter;
+     xmlRMutexPtr mutex;
  
      struct _xmlDictEntry *dict;
      int size;
***************
*** 288,295 ****
        dict->strings = NULL;
        dict->subdict = NULL;
          if (dict->dict) {
!           memset(dict->dict, 0, MIN_DICT_SIZE * sizeof(xmlDictEntry));
!           return(dict);
          }
          xmlFree(dict);
      }
--- 289,299 ----
        dict->strings = NULL;
        dict->subdict = NULL;
          if (dict->dict) {
!             if ((dict->mutex = xmlNewRMutex()) != NULL) {
!                 memset(dict->dict, 0, MIN_DICT_SIZE * sizeof(xmlDictEntry));
!                 return(dict);
!             }
!             xmlFree(dict->dict);
          }
          xmlFree(dict);
      }
***************
*** 329,335 ****
--- 333,341 ----
  int
  xmlDictReference(xmlDictPtr dict) {
      if (dict == NULL) return -1;
+     xmlRMutexLock(dict->mutex);
      dict->ref_counter++;
+     xmlRMutexUnlock(dict->mutex);
      return(0);
  }
  
***************
*** 446,453 ****
        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);
--- 452,465 ----
        return;
  
      /* decrement the counter, it may be shared by a parser and docs */
+     xmlRMutexLock(dict->mutex);
      dict->ref_counter--;
!     if (dict->ref_counter > 0) {
!         xmlRMutexUnlock(dict->mutex);
!         return;
!     }
! 
!     xmlRMutexUnlock(dict->mutex);
  
      if (dict->subdict != NULL) {
          xmlDictFree(dict->subdict);
***************
*** 477,482 ****
--- 489,495 ----
        xmlFree(pool);
        pool = nextp;
      }
+     xmlFreeRMutex(dict->mutex);
      xmlFree(dict);
  }