I have attached the patch.
With the help of Cedric I have made this patch.
2 mutexs are used, one for the strings with a size < 4 and one for the
others.
2 macros has been updated to add the possibility to unlock a mutex if
necessary.
In this version the mutexs are activated if eina is installed with the
pthread support ( I use the same code than eina_log ). If you wish I can add
a function to manually active the mutexs.
The patch is simple, this was not a big job to do. Helgrind reports no
errors with my application and I use a lot of eina stringshare + threads
(without the patch my application segv after eina reports some invalids use
of eina stringshare). Of course I do not use all the combinations of calls.
E17 works too.
2009/10/26 Cedric BAIL <cedric.b...@free.fr>
> On Mon, Oct 26, 2009 at 1:07 PM, Atton Jonathan
> <jonathan.at...@gmail.com> wrote:
> > actually all eina should be thread safe. Maybe eet and ecore too. If you
> are
> > not against this I will start to work on eina.
>
> Actually, I think we should do this really carefully. I think we
> should make stringshare thread safe, because their is no way from an
> user API point of view to use them without messing everything from
> another thread.
>
> So i agree we will need something like 'int
> eina_thread_safe_init(void)' and 'int
> eina_thread_safe_shutdown(void)', that track the number of time an
> user need to be safe and set one global flag that activate some mutex
> for stringshare. But I don't want to see eina_list or eina_hash, add
> some pthread lock just because the user can't do it by itself. And in
> most case, he will be able to do it much more efficiently than we
> could in eina.
>
> > 2009/10/25 Gustavo Sverzut Barbieri <barbi...@profusion.mobi>
> >> On Sun, Oct 25, 2009 at 5:20 PM, Atton Jonathan
> >> <jonathan.at...@gmail.com> wrote:
> >> > hey,
> >> >
> >> > I talked a week ago with cedric about making eina stringshare thread
> >> safe.
> >> >
> >> > The mutex could be activated/deactivated with a classic function and
> then
> >> 2
> >> > macros will be use:
> >> >
> >> > LOCK : if(mutex_activated) pthread_mutex_lock()
> >> > UNLOCK : if(mutex_activated) pthread_mutex_unlock()
> >> >
> >> > By default the mutex can be deactivated and the developer activate it
> if
> >> > necessary.
> >> >
> >> > What do you think about this idea ?
> >>
> >> The only problem with this is inconsistency. Okay, we have stringshare
> >> as this, but still default mempool is not so you cannot allocate list
> >> nodes... then you cannot add ecore idlers/timers/pollers/... from
> >> threads, and so on. Inconsistency is bad :-(
>
> The default mempool is thread safe. Definitivly, it should work safely
> if you build eina with pthread support. If not, it's a bug that should
> be fixed. For ecore, this is another matter, right now, by design we
> don't want to
>
> >> I really don't see any clear solution to this problem. Maybe we could
> >> have something like glib's "threads init". I have this for logging,
> >> but i was really reluctant to add it. But if so, then we must make
> >> sure all types would be thread safe.
>
> So maybe when thread safe is required do the same trick for eina_log to.
> --
> Cedric BAIL
>
--
Regards.
Index: src/lib/eina_stringshare.c
===================================================================
--- src/lib/eina_stringshare.c (revision 43297)
+++ src/lib/eina_stringshare.c (working copy)
@@ -100,19 +100,23 @@
static const char EINA_MAGIC_STRINGSHARE_NODE_STR[] = "Eina Stringshare Node";
-#define EINA_MAGIC_CHECK_STRINGSHARE_HEAD(d, ...) \
+#define EINA_MAGIC_CHECK_STRINGSHARE_HEAD(d, unlock, ...) \
do { \
if (!EINA_MAGIC_CHECK((d), EINA_MAGIC_STRINGSHARE_HEAD)) \
{ \
EINA_MAGIC_FAIL((d), EINA_MAGIC_STRINGSHARE_HEAD); \
+ unlock; \
return __VA_ARGS__; \
} \
} while (0);
-#define EINA_MAGIC_CHECK_STRINGSHARE_NODE(d) \
+#define EINA_MAGIC_CHECK_STRINGSHARE_NODE(d, unlock) \
do { \
if (!EINA_MAGIC_CHECK((d), EINA_MAGIC_STRINGSHARE_NODE)) \
+ { \
+ unlock; \
EINA_MAGIC_FAIL((d), EINA_MAGIC_STRINGSHARE_NODE); \
+ } \
} while (0);
typedef struct _Eina_Stringshare Eina_Stringshare;
@@ -170,6 +174,48 @@
#endif
#define DBG(...) EINA_LOG_DOM_DBG(_eina_stringshare_log_dom, __VA_ARGS__)
+
+
+#ifdef EFL_HAVE_PTHREAD
+#include <pthread.h>
+static Eina_Bool _threads_enabled = EINA_FALSE;
+static pthread_t _main_thread;
+
+#define IS_MAIN(t) pthread_equal(t, _main_thread)
+#define IS_OTHER(t) EINA_UNLIKELY(!IS_MAIN(t))
+#define CHECK_MAIN(...)
\
+ do { \
+ if (!IS_MAIN(pthread_self())) { \
+ fprintf(stderr, \
+ "ERR: not main thread! current=%lu, main=%lu\n", \
+ pthread_self(), _main_thread); \
+ return __VA_ARGS__; \
+ } \
+ } while (0)
+
+//string < 4
+static pthread_mutex_t _mutex_small = PTHREAD_MUTEX_INITIALIZER;
+//string >= 4
+static pthread_mutex_t _mutex_big = PTHREAD_MUTEX_INITIALIZER;
+#define LOCK_SMALL() pthread_mutex_lock(&_mutex_small);
+#define UNLOCK_SMALL() pthread_mutex_unlock(&_mutex_small);
+#define LOCK_BIG() pthread_mutex_lock(&_mutex_big);
+#define UNLOCK_BIG() pthread_mutex_unlock(&_mutex_big);
+#define _INIT() do {} while (0)
+#define _SHUTDOWN() do {} while (0)
+#else
+#define LOCK_SMALL() do {} while (0)
+#define UNLOCK_SMALL() do {} while (0)
+#define LOCK_BIG() do {} while (0)
+#define UNLOCK_BIG() do {} while (0)
+#define IS_MAIN(t) (1)
+#define IS_OTHER(t) (0)
+#define CHECK_MAIN(...) do {} while (0)
+#define INIT() do {} while (0)
+#define SHUTDOWN() do {} while (0)
+#endif
+
+
static const unsigned char _eina_stringshare_single[512] = {
0,0,1,0,2,0,3,0,4,0,5,0,6,0,7,0,8,0,9,0,10,0,11,0,12,0,13,0,14,0,15,0,
16,0,17,0,18,0,19,0,20,0,21,0,22,0,23,0,24,0,25,0,26,0,27,0,28,0,29,0,30,0,
@@ -276,6 +322,9 @@
static void
_eina_stringshare_population_add(int slen)
{
+ LOCK_SMALL();
+ LOCK_BIG();
+
population.count++;
if (population.count > population.max)
population.max = population.count;
@@ -286,14 +335,23 @@
if (population_group[slen].count > population_group[slen].max)
population_group[slen].max = population_group[slen].count;
}
+
+ UNLOCK_BIG();
+ UNLOCK_SMALL();
}
static void
_eina_stringshare_population_del(int slen)
{
+ LOCK_SMALL();
+ LOCK_BIG();
+
population.count--;
if (slen < 4)
population_group[slen].count--;
+
+ UNLOCK_BIG();
+ UNLOCK_SMALL();
}
static void
@@ -331,7 +389,7 @@
static int
_eina_stringshare_cmp(const Eina_Stringshare_Head *ed, const int *hash,
__UNUSED__ int length, __UNUSED__ void *data)
{
- EINA_MAGIC_CHECK_STRINGSHARE_HEAD(ed, 0);
+ EINA_MAGIC_CHECK_STRINGSHARE_HEAD(ed, , 0);
return ed->hash - *hash;
}
@@ -339,8 +397,8 @@
static Eina_Rbtree_Direction
_eina_stringshare_node(const Eina_Stringshare_Head *left, const
Eina_Stringshare_Head *right, __UNUSED__ void *data)
{
- EINA_MAGIC_CHECK_STRINGSHARE_HEAD(left, 0);
- EINA_MAGIC_CHECK_STRINGSHARE_HEAD(right, 0);
+ EINA_MAGIC_CHECK_STRINGSHARE_HEAD(left, , 0);
+ EINA_MAGIC_CHECK_STRINGSHARE_HEAD(right, , 0);
if (left->hash - right->hash < 0)
return EINA_RBTREE_LEFT;
@@ -350,7 +408,7 @@
static void
_eina_stringshare_head_free(Eina_Stringshare_Head *ed, __UNUSED__ void *data)
{
- EINA_MAGIC_CHECK_STRINGSHARE_HEAD(ed);
+ EINA_MAGIC_CHECK_STRINGSHARE_HEAD(ed, );
while (ed->head)
{
@@ -851,6 +909,11 @@
return EINA_FALSE;
}
+#ifdef EFL_HAVE_PTHREAD
+ _main_thread = pthread_self();
+ _INIT();
+#endif
+
#define EMS(n) eina_magic_string_static_set(n, n##_STR)
EMS(EINA_MAGIC_STRINGSHARE);
EMS(EINA_MAGIC_STRINGSHARE_HEAD);
@@ -879,6 +942,9 @@
{
unsigned int i;
+ LOCK_SMALL();
+ LOCK_BIG();
+
_eina_stringshare_population_stats();
/* remove any string still in the table */
@@ -893,6 +959,15 @@
_eina_stringshare_small_shutdown();
eina_log_domain_unregister(_eina_stringshare_log_dom);
_eina_stringshare_log_dom = -1;
+
+ UNLOCK_BIG();
+ UNLOCK_SMALL();
+#ifdef EFL_HAVE_PTHREAD
+ _SHUTDOWN();
+ _threads_enabled = 0;
+#endif
+
+
return EINA_TRUE;
}
@@ -936,36 +1011,53 @@
else if (slen == 1)
return (const char *)_eina_stringshare_single + ((*str) << 1);
else if (slen < 4)
- return _eina_stringshare_small_add(str, slen);
+ {
+ LOCK_SMALL();
+ const char *s = _eina_stringshare_small_add(str, slen);
+ UNLOCK_SMALL();
+ return s;
+ }
hash = eina_hash_superfast(str, slen);
hash_num = hash & 0xFF;
hash = (hash >> 8) & EINA_STRINGSHARE_MASK;
+ LOCK_BIG();
p_bucket = share->buckets + hash_num;
+
ed = _eina_stringshare_find_hash(*p_bucket, hash);
if (!ed)
- return _eina_stringshare_add_head(p_bucket, hash, str, slen);
+ {
+ const char *s = _eina_stringshare_add_head(p_bucket, hash, str, slen);
+ UNLOCK_BIG();
+ return s;
+ }
- EINA_MAGIC_CHECK_STRINGSHARE_HEAD(ed, NULL);
+ EINA_MAGIC_CHECK_STRINGSHARE_HEAD(ed, UNLOCK_BIG(), NULL);
el = _eina_stringshare_head_find(ed, str, slen);
if (el)
{
- EINA_MAGIC_CHECK_STRINGSHARE_NODE(el);
+ EINA_MAGIC_CHECK_STRINGSHARE_NODE(el, UNLOCK_BIG());
el->references++;
+ UNLOCK_BIG();
return el->str;
}
el = _eina_stringshare_node_alloc(slen);
if (!el)
- return NULL;
+ {
+ UNLOCK_BIG();
+ return NULL;
+ }
_eina_stringshare_node_init(el, str, slen);
el->next = ed->head;
ed->head = el;
_eina_stringshare_population_head_add(ed);
+ UNLOCK_BIG();
+
return el->str;
}
@@ -1014,7 +1106,7 @@
const size_t offset = (char *)&(t.str) - (char *)&t;
node = (Eina_Stringshare_Node *)(str - offset);
- EINA_MAGIC_CHECK_STRINGSHARE_NODE(node);
+ EINA_MAGIC_CHECK_STRINGSHARE_NODE(node, );
return node;
}
@@ -1047,18 +1139,27 @@
if (slen < 2)
{
_eina_stringshare_population_add(slen);
+
return str;
}
else if (slen < 4)
{
_eina_stringshare_population_add(slen);
- return _eina_stringshare_small_add(str, slen);
+
+ LOCK_SMALL();
+ const char *s = _eina_stringshare_small_add(str, slen);
+ UNLOCK_SMALL();
+
+ return s;
}
+ LOCK_BIG();
node = _eina_stringshare_node_from_str(str);
node->references++;
DBG("str=%p (%s) refs=%u", str, str, node->references);
+ UNLOCK_BIG();
+
_eina_stringshare_population_add(node->length);
return str;
@@ -1102,15 +1203,20 @@
return;
else if (slen < 4)
{
+ LOCK_SMALL();
_eina_stringshare_small_del(str, slen);
+ UNLOCK_SMALL();
return;
}
+ LOCK_BIG();
+
node = _eina_stringshare_node_from_str(str);
if (node->references > 1)
{
node->references--;
DBG("str=%p (%s) refs=%u", str, str, node->references);
+ UNLOCK_BIG();
return;
}
@@ -1127,7 +1233,7 @@
if (!ed)
goto on_error;
- EINA_MAGIC_CHECK_STRINGSHARE_HEAD(ed);
+ EINA_MAGIC_CHECK_STRINGSHARE_HEAD(ed, UNLOCK_BIG());
if (!_eina_stringshare_head_remove_node(ed, node))
goto on_error;
@@ -1140,9 +1246,12 @@
else
_eina_stringshare_population_head_del(ed);
+ UNLOCK_BIG();
+
return;
on_error:
+ UNLOCK_BIG();
/* possible segfault happened before here, but... */
CRITICAL("EEEK trying to del non-shared stringshare \"%s\"", str);
}
@@ -1231,7 +1340,10 @@
eina_iterator_array_check(const Eina_Rbtree *rbtree __UNUSED__,
Eina_Stringshare_Head *head, struct dumpinfo *fdata)
{
Eina_Stringshare_Node *node;
-
+
+ LOCK_SMALL();
+ LOCK_BIG();
+
fdata->used += sizeof(Eina_Stringshare_Head);
for (node = head->head; node; node = node->next)
{
@@ -1243,6 +1355,10 @@
fdata->dups += node->references - 1;
fdata->unique++;
}
+
+ UNLOCK_BIG();
+ UNLOCK_SMALL();
+
return EINA_TRUE;
}
@@ -1266,7 +1382,12 @@
di.unique = 0;
printf("DDD: len ref string\n");
printf("DDD:-------------------\n");
+
+ LOCK_SMALL();
_eina_stringshare_small_dump(&di);
+ UNLOCK_SMALL();
+
+ LOCK_BIG();
for (i = 0; i < EINA_STRINGSHARE_BUCKETS; i++)
{
if (!share->buckets[i]) continue;
@@ -1294,8 +1415,11 @@
for (i = 0; i < sizeof (population_group) / sizeof (population_group[0]);
++i)
fprintf(stderr, "DDD: %i strings of length %i, max strings: %i\n",
population_group[i].count, i, population_group[i].max);
#endif
+
+ UNLOCK_BIG();
}
/**
* @}
*/
+
------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
