Hi Ray,
thanks for Clone. It's highly useful!
Unfortunately, Clone isn't thread safe because it uses a global C-level
HV *HSEEN.
I'm involved in the development of the Padre editor and we have to use
threads for concurrency. Our dependencies use Clone, and so do we.
Please consider applying the patch (against Clone 0.29) below and making
a quick release. If I can be of any assistance, please let me know. I'm
prepared to go at as far as making the release for you if you let me.
The appended patch replaces the global HSEEN with an HV* which is
created in the clone XSUB and passed as an argument to the various
*_clone functions. Naturally, this comes with a slight performance
penalty. Some benchmarking suggested between 2-3%. But it's better to be
correct than fast.
Best regards,
Steffen
| Subject: | Clone-thread-saftey.patch |
--- Clone-0.29/Clone.xs 2008-07-12 18:49:01.000000000 +0200
+++ C2/Clone.xs 2008-12-08 20:35:01.000000000 +0100
@@ -10,9 +10,9 @@
#define CLONE_STORE(x,y) \
do { \
- if (!hv_store(HSEEN, CLONE_KEY(x), PTRSIZE, SvREFCNT_inc(y), 0)) { \
+ if (!hv_store(hseen, CLONE_KEY(x), PTRSIZE, SvREFCNT_inc(y), 0)) { \
SvREFCNT_dec(y); /* Restore the refcount */ \
- croak("Can't store clone in seen hash (HSEEN)"); \
+ croak("Can't store clone in seen hash (hseen)"); \
} \
else { \
TRACEME(("storing ref = 0x%x clone = 0x%x\n", ref, clone)); \
@@ -21,14 +21,12 @@
} \
} while (0)
-#define CLONE_FETCH(x) (hv_fetch(HSEEN, CLONE_KEY(x), PTRSIZE, 0))
+#define CLONE_FETCH(x) (hv_fetch(hseen, CLONE_KEY(x), PTRSIZE, 0))
-static SV *hv_clone (SV *, SV *, int);
-static SV *av_clone (SV *, SV *, int);
-static SV *sv_clone (SV *, int);
-static SV *rv_clone (SV *, int);
-
-static HV *HSEEN;
+static SV *hv_clone (SV *, SV *, HV *, int);
+static SV *av_clone (SV *, SV *, HV *, int);
+static SV *sv_clone (SV *, HV *, int);
+static SV *rv_clone (SV *, HV *, int);
#ifdef DEBUG_CLONE
#define TRACEME(a) printf("%s:%d: ",__FUNCTION__, __LINE__) && printf a;
@@ -37,7 +35,7 @@
#endif
static SV *
-hv_clone (SV * ref, SV * target, int depth)
+hv_clone (SV * ref, SV * target, HV* hseen, int depth)
{
HV *clone = (HV *) target;
HV *self = (HV *) ref;
@@ -53,7 +51,7 @@
{
SV *key = hv_iterkeysv (next);
hv_store_ent (clone, key,
- sv_clone (hv_iterval (self, next), recur), 0);
+ sv_clone (hv_iterval (self, next), hseen, recur), 0);
}
TRACEME(("clone = 0x%x(%d)\n", clone, SvREFCNT(clone)));
@@ -61,7 +59,7 @@
}
static SV *
-av_clone (SV * ref, SV * target, int depth)
+av_clone (SV * ref, SV * target, HV* hseen, int depth)
{
AV *clone = (AV *) target;
AV *self = (AV *) ref;
@@ -87,7 +85,7 @@
{
svp = av_fetch (self, i, 0);
if (svp)
- av_store (clone, i, sv_clone (*svp, recur));
+ av_store (clone, i, sv_clone (*svp, hseen, recur));
}
TRACEME(("clone = 0x%x(%d)\n", clone, SvREFCNT(clone)));
@@ -95,7 +93,7 @@
}
static SV *
-rv_clone (SV * ref, int depth)
+rv_clone (SV * ref, HV* hseen, int depth)
{
SV *clone = NULL;
SV *rv = NULL;
@@ -109,18 +107,18 @@
if (sv_isobject (ref))
{
- clone = newRV_noinc(sv_clone (SvRV(ref), depth));
+ clone = newRV_noinc(sv_clone (SvRV(ref), hseen, depth));
sv_2mortal (sv_bless (clone, SvSTASH (SvRV (ref))));
}
else
- clone = newRV_inc(sv_clone (SvRV(ref), depth));
+ clone = newRV_inc(sv_clone (SvRV(ref), hseen, depth));
TRACEME(("clone = 0x%x(%d)\n", clone, SvREFCNT(clone)));
return clone;
}
static SV *
-sv_clone (SV * ref, int depth)
+sv_clone (SV * ref, HV* hseen, int depth)
{
SV *clone = ref;
SV **seen = NULL;
@@ -252,7 +250,7 @@
continue;
break;
default:
- obj = sv_clone(mg->mg_obj, -1);
+ obj = sv_clone(mg->mg_obj, hseen, -1);
}
} else {
TRACEME(("magic object for type %c in NULL\n", mg->mg_type));
@@ -275,15 +273,15 @@
;;
}
else if ( SvTYPE(ref) == SVt_PVHV )
- clone = hv_clone (ref, clone, depth);
+ clone = hv_clone (ref, clone, hseen, depth);
else if ( SvTYPE(ref) == SVt_PVAV )
- clone = av_clone (ref, clone, depth);
+ clone = av_clone (ref, clone, hseen, depth);
/* 3: REFERENCE (inlined for speed) */
else if (SvROK (ref))
{
TRACEME(("clone = 0x%x(%d)\n", clone, SvREFCNT(clone)));
SvREFCNT_dec(SvRV(clone));
- SvRV(clone) = sv_clone (SvRV(ref), depth); /* Clone the referent */
+ SvRV(clone) = sv_clone (SvRV(ref), hseen, depth); /* Clone the referent */
if (sv_isobject (ref))
{
sv_bless (clone, SvSTASH (SvRV (ref)));
@@ -301,10 +299,6 @@
PROTOTYPES: ENABLE
-BOOT:
-/* Initialize HSEEN */
-HSEEN = newHV(); if (!HSEEN) croak ("Can't initialize seen hash (HSEEN)");
-
void
clone(self, depth=-1)
SV *self
@@ -312,8 +306,10 @@
PREINIT:
SV * clone = &PL_sv_undef;
PPCODE:
+ HV *hseen = newHV();
TRACEME(("ref = 0x%x\n", self));
- clone = sv_clone(self, depth);
- hv_clear(HSEEN); /* Free HV */
+ clone = sv_clone(self, hseen, depth);
+ hv_clear(hseen); /* Free HV */
+ SvREFCNT_dec((SV *)hseen);
EXTEND(SP,1);
PUSHs(sv_2mortal(clone));