improve LocalizeFormat, internal caching of localized formats inside msgcat for locale and format objects

smart reference introduced in dict (smart pointer with 0 object reference but increase dict-reference, provide changeable locale dict)
code review

5 files changed, 214 insertions, 190 deletions

diff --git a/generic/tclClock.c b/generic/tclClock.c
index e066812..08bc6ef 100644
--- a/generic/tclClock.c
+++ b/generic/tclClock.c
@@ -42,53 +42,7 @@ static const int daysInPriorMonths[2][13] = {
  * Enumeration of the string literals used in [clock]
  */
 
-typedef enum ClockLiteral {
-    LIT__NIL,
-    LIT__DEFAULT_FORMAT,
-    LIT_BCE,		LIT_C,
-    LIT_CANNOT_USE_GMT_AND_TIMEZONE,
-    LIT_CE,
-    LIT_DAYOFMONTH,	LIT_DAYOFWEEK,		LIT_DAYOFYEAR,
-    LIT_ERA,		LIT_GMT,		LIT_GREGORIAN,
-    LIT_INTEGER_VALUE_TOO_LARGE,
-    LIT_ISO8601WEEK,	LIT_ISO8601YEAR,
-    LIT_JULIANDAY,	LIT_LOCALSECONDS,
-    LIT_MONTH,
-    LIT_SECONDS,	LIT_TZNAME,		LIT_TZOFFSET,
-    LIT_YEAR,
-    LIT_TZDATA,
-    LIT_GETSYSTEMTIMEZONE,
-    LIT_SETUPTIMEZONE,
-    LIT_MCGET,		LIT_TCL_CLOCK,
-    LIT_LOCALIZE_FORMAT,
-#if 0
-    LIT_FREESCAN,
-#endif
-    LIT__END
-} ClockLiteral;
-static const char *const Literals[] = {
-    "",
-    "%a %b %d %H:%M:%S %Z %Y",
-    "BCE",		"C",
-    "cannot use -gmt and -timezone in same call",
-    "CE",
-    "dayOfMonth",	"dayOfWeek",		"dayOfYear",
-    "era",		":GMT",			"gregorian",
-    "integer value too large to represent",
-    "iso8601Week",	"iso8601Year",
-    "julianDay",	"localSeconds",
-    "month",
-    "seconds",		"tzName",		"tzOffset",
-    "year",
-    "::tcl::clock::TZData",
-    "::tcl::clock::GetSystemTimeZone",
-    "::tcl::clock::SetupTimeZone",
-    "::msgcat::mcget", "::tcl::clock",
-    "::tcl::clock::LocalizeFormat"
-#if 0
-    "::tcl::clock::FreeScan"
-#endif
-};
+CLOCK_LITERAL_ARRAY(Literals);
 
 /* Msgcat literals for exact match (mcKey) */
 CLOCK_LOCALE_LITERAL_ARRAY(MsgCtLiterals, "");
@@ -634,56 +588,6 @@ ClockMCGetListIdxDict(
     return valObj;
 }
 
-MODULE_SCOPE Tcl_Obj *
-ClockLocalizeFormat(
-    ClockFmtScnCmdArgs *opts)
-{
-    ClockClientData *dataPtr = opts->clientData;
-    Tcl_Obj *valObj, *keyObj;
-
-    keyObj = ClockFrmObjGetLocFmtKey(opts->interp, opts->formatObj);
-
-    if (opts->mcDictObj == NULL) {
-	ClockMCDict(opts);
-	if (opts->mcDictObj == NULL)
-	    return NULL;
-    }
-
-    /* try to find in cache within mc-catalog */
-    if (Tcl_DictObjGet(NULL, opts->mcDictObj, 
-	    keyObj, &valObj) != TCL_OK) {
-	return NULL;
-    }
-    if (valObj == NULL) {
-	Tcl_Obj *callargs[4];
-	/* call LocalizeFormat locale format fmtkey */
-	callargs[0] = dataPtr->literals[LIT_LOCALIZE_FORMAT];
-	callargs[1] = opts->localeObj;
-	callargs[2] = opts->formatObj;
-	callargs[3] = keyObj;
-	Tcl_IncrRefCount(keyObj);
-	if (Tcl_EvalObjv(opts->interp, 4, callargs, 0) != TCL_OK
-	) {
-	    Tcl_DecrRefCount(keyObj);
-	    return NULL;
-	}
-
-	valObj = Tcl_GetObjResult(opts->interp);
-
-	if (Tcl_DictObjPut(opts->interp, opts->mcDictObj,
-		keyObj, valObj) != TCL_OK
-	) {
-	    Tcl_DecrRefCount(keyObj);
-	    return NULL;
-	}
-
-	Tcl_DecrRefCount(keyObj);
-	Tcl_ResetResult(opts->interp);
-    }
-
-    return (opts->formatObj = valObj);
-}
-
 /*
  *----------------------------------------------------------------------
  */
diff --git a/generic/tclClockFmt.c b/generic/tclClockFmt.c
index 184b52a..daedb26 100644
--- a/generic/tclClockFmt.c
+++ b/generic/tclClockFmt.c
@@ -233,65 +233,6 @@ static Tcl_HashKeyType ClockFmtScnStorageHashKeyType;
 
 
 /*
- *----------------------------------------------------------------------
- */
-
-static ClockFmtScnStorage *
-FindOrCreateFmtScnStorage(
-    Tcl_Interp *interp, 
-    const char *strFmt)
-{
-    ClockFmtScnStorage *fss = NULL;
-    int new;
-    Tcl_HashEntry *hPtr;
-
-    Tcl_MutexLock(&ClockFmtMutex);
-
-    /* if not yet initialized */
-    if (!initialized) {
-	/* initialize type */
-	memcpy(&ClockFmtScnStorageHashKeyType, &tclStringHashKeyType, sizeof(tclStringHashKeyType));
-	ClockFmtScnStorageHashKeyType.allocEntryProc = ClockFmtScnStorageAllocProc;
-	ClockFmtScnStorageHashKeyType.freeEntryProc = ClockFmtScnStorageFreeProc;
-
-	/* initialize hash table */
-	Tcl_InitCustomHashTable(&FmtScnHashTable, TCL_CUSTOM_TYPE_KEYS,
-	    &ClockFmtScnStorageHashKeyType);
-
-	initialized = 1;
-	Tcl_CreateExitHandler(ClockFrmScnFinalize, NULL);
-    }
-
-    /* get or create entry (and alocate storage) */
-    hPtr = Tcl_CreateHashEntry(&FmtScnHashTable, strFmt, &new);
-    if (hPtr != NULL) {
-	
-	fss = FmtScn4HashEntry(hPtr);
-
-	#if CLOCK_FMT_SCN_STORAGE_GC_SIZE > 0
-	  /* unlink if it is currently in GC */
-	  if (new == 0 && fss->objRefCount == 0) {
-	      ClockFmtScnStorage_GC_Out(fss);
-	  }
-	#endif
-
-	/* new reference, so increment in lock right now */
-	fss->objRefCount++;
-    }
-
-    Tcl_MutexUnlock(&ClockFmtMutex);
-
-    if (fss == NULL && interp != NULL) {
-	Tcl_AppendResult(interp, "retrieve clock format failed \"",
-	    strFmt ? strFmt : "", "\"", NULL);
-	Tcl_SetErrorCode(interp, "TCL", "EINVAL", NULL);
-    }
-
-    return fss;
-}
-
-
-/*
  * Type definition.
  */
 
@@ -303,20 +244,11 @@ Tcl_ObjType ClockFmtObjType = {
     ClockFmtObj_SetFromAny	 /* setFromAnyProc */
 };
 
-#define SetObjClockFmtScn(objPtr, fss) \
-    objPtr->internalRep.twoPtrValue.ptr1 = fss
 #define ObjClockFmtScn(objPtr) \
-    (ClockFmtScnStorage *)objPtr->internalRep.twoPtrValue.ptr1;
+    (*((ClockFmtScnStorage **)&(objPtr)->internalRep.twoPtrValue.ptr1))
 
-#define SetObjLocFmtKey(objPtr, key) \
-    Tcl_InitObjRef((Tcl_Obj *)objPtr->internalRep.twoPtrValue.ptr2, key)
 #define ObjLocFmtKey(objPtr) \
-    ((Tcl_Obj *)objPtr->internalRep.twoPtrValue.ptr2)
-
-#define ClockFmtObj_SetObjIntRep(objPtr, fss, key) \
-    objPtr->internalRep.twoPtrValue.ptr1 = fss; \
-    Tcl_InitObjRef((Tcl_Obj *)objPtr->internalRep.twoPtrValue.ptr2, key); \
-    objPtr->typePtr = &ClockFmtObjType;
+    (*((Tcl_Obj **)&(objPtr)->internalRep.twoPtrValue.ptr2))
 
 /*
  *----------------------------------------------------------------------
@@ -334,7 +266,15 @@ ClockFmtObj_DupInternalRep(srcPtr, copyPtr)
 	Tcl_MutexUnlock(&ClockFmtMutex);
     }
 
-    ClockFmtObj_SetObjIntRep(copyPtr, fss, ObjLocFmtKey(srcPtr));
+    ObjClockFmtScn(copyPtr) = fss;
+    /* regards special case - format not localizable */
+    if (ObjLocFmtKey(srcPtr) != srcPtr) {
+	Tcl_InitObjRef(ObjLocFmtKey(copyPtr), ObjLocFmtKey(srcPtr));
+    } else {
+	ObjLocFmtKey(copyPtr) = copyPtr;
+    }
+    copyPtr->typePtr = &ClockFmtObjType;
+
 
     /* if no format representation, dup string representation */
     if (fss == NULL) {
@@ -365,8 +305,12 @@ ClockFmtObj_FreeInternalRep(objPtr)
 	}
 	Tcl_MutexUnlock(&ClockFmtMutex);
     }
-    SetObjClockFmtScn(objPtr, NULL);
-    Tcl_UnsetObjRef(ObjLocFmtKey(objPtr));
+    ObjClockFmtScn(objPtr) = NULL;
+    if (ObjLocFmtKey(objPtr) != objPtr) {
+	Tcl_UnsetObjRef(ObjLocFmtKey(objPtr));
+    } else {
+	ObjLocFmtKey(objPtr) = NULL;
+    }
     objPtr->typePtr = NULL;
 };
 /*
@@ -385,8 +329,8 @@ ClockFmtObj_SetFromAny(interp, objPtr)
 	objPtr->typePtr->freeIntRepProc(objPtr);
 
     /* initial state of format object */
-    objPtr->internalRep.twoPtrValue.ptr1 = NULL;
-    objPtr->internalRep.twoPtrValue.ptr2 = NULL;
+    ObjClockFmtScn(objPtr) = NULL;
+    ObjLocFmtKey(objPtr) = NULL;
     objPtr->typePtr = &ClockFmtObjType;
 
     return TCL_OK;
@@ -435,13 +379,74 @@ ClockFrmObjGetLocFmtKey(
     }
 
     keyObj = Tcl_ObjPrintf("FMT_%s", TclGetString(objPtr));
-    SetObjLocFmtKey(objPtr, keyObj);
+    Tcl_InitObjRef(ObjLocFmtKey(objPtr), keyObj);
 
     return keyObj;
 }
 
 /*
  *----------------------------------------------------------------------
+ */
+
+static ClockFmtScnStorage *
+FindOrCreateFmtScnStorage(
+    Tcl_Interp *interp, 
+    Tcl_Obj    *objPtr)
+{
+    const char *strFmt = TclGetString(objPtr);
+    ClockFmtScnStorage *fss = NULL;
+    int new;
+    Tcl_HashEntry *hPtr;
+
+    Tcl_MutexLock(&ClockFmtMutex);
+
+    /* if not yet initialized */
+    if (!initialized) {
+	/* initialize type */
+	memcpy(&ClockFmtScnStorageHashKeyType, &tclStringHashKeyType, sizeof(tclStringHashKeyType));
+	ClockFmtScnStorageHashKeyType.allocEntryProc = ClockFmtScnStorageAllocProc;
+	ClockFmtScnStorageHashKeyType.freeEntryProc = ClockFmtScnStorageFreeProc;
+
+	/* initialize hash table */
+	Tcl_InitCustomHashTable(&FmtScnHashTable, TCL_CUSTOM_TYPE_KEYS,
+	    &ClockFmtScnStorageHashKeyType);
+
+	initialized = 1;
+	Tcl_CreateExitHandler(ClockFrmScnFinalize, NULL);
+    }
+
+    /* get or create entry (and alocate storage) */
+    hPtr = Tcl_CreateHashEntry(&FmtScnHashTable, strFmt, &new);
+    if (hPtr != NULL) {
+	
+	fss = FmtScn4HashEntry(hPtr);
+
+	#if CLOCK_FMT_SCN_STORAGE_GC_SIZE > 0
+	  /* unlink if it is currently in GC */
+	  if (new == 0 && fss->objRefCount == 0) {
+	      ClockFmtScnStorage_GC_Out(fss);
+	  }
+	#endif
+
+	/* new reference, so increment in lock right now */
+	fss->objRefCount++;
+
+	ObjClockFmtScn(objPtr) = fss;
+    }
+
+    Tcl_MutexUnlock(&ClockFmtMutex);
+
+    if (fss == NULL && interp != NULL) {
+	Tcl_AppendResult(interp, "retrieve clock format failed \"",
+	    strFmt ? strFmt : "", "\"", NULL);
+	Tcl_SetErrorCode(interp, "TCL", "EINVAL", NULL);
+    }
+
+    return fss;
+}
+
+/*
+ *----------------------------------------------------------------------
  *
  * Tcl_GetClockFrmScnFromObj --
  *
@@ -475,8 +480,7 @@ Tcl_GetClockFrmScnFromObj(
     fss = ObjClockFmtScn(objPtr);
 
     if (fss == NULL) {
-	const char *strFmt = TclGetString(objPtr);
-	fss = FindOrCreateFmtScnStorage(interp, strFmt);
+	fss = FindOrCreateFmtScnStorage(interp, objPtr);
     }
 
     return fss;
@@ -772,7 +776,7 @@ ClockGetOrParseScanFormat(
     fss = ObjClockFmtScn(formatObj);
 
     if (fss == NULL) {
-	fss = FindOrCreateFmtScnStorage(interp, TclGetString(formatObj));
+	fss = FindOrCreateFmtScnStorage(interp, formatObj);
 	if (fss == NULL) {
 	    return NULL;
 	}
@@ -898,6 +902,72 @@ done:
     return fss->scnTok;
 }
 
+MODULE_SCOPE Tcl_Obj *
+ClockLocalizeFormat(
+    ClockFmtScnCmdArgs *opts)
+{
+    ClockClientData *dataPtr = opts->clientData;
+    Tcl_Obj *valObj = NULL, *keyObj;
+
+    keyObj = ClockFrmObjGetLocFmtKey(opts->interp, opts->formatObj);
+
+    /* special case - format object is not localizable */
+    if (keyObj == opts->formatObj) {
+	return opts->formatObj;
+    }
+
+    if (opts->mcDictObj == NULL) {
+	ClockMCDict(opts);
+	if (opts->mcDictObj == NULL)
+	    return NULL;
+    }
+
+    /* try to find in cache within locale mc-catalog */
+    if (Tcl_DictObjGet(NULL, opts->mcDictObj, 
+	    keyObj, &valObj) != TCL_OK) {
+	return NULL;
+    }
+
+    /* call LocalizeFormat locale format fmtkey */
+    if (valObj == NULL) {
+	Tcl_Obj *callargs[4];
+	callargs[0] = dataPtr->literals[LIT_LOCALIZE_FORMAT];
+	callargs[1] = opts->localeObj;
+	callargs[2] = opts->formatObj;
+	callargs[3] = keyObj;
+	Tcl_IncrRefCount(keyObj);
+	if (Tcl_EvalObjv(opts->interp, 4, callargs, 0) != TCL_OK
+	) {
+	    goto clean;
+	}
+
+	valObj = Tcl_GetObjResult(opts->interp);
+
+	/* cache it inside mc-dictionary (this incr. ref count of keyObj/valObj) */
+	if (Tcl_DictObjPut(opts->interp, opts->mcDictObj,
+		keyObj, valObj) != TCL_OK
+	) {
+	    valObj = NULL;
+	    goto clean;
+	}
+
+	/* check special case - format object is not localizable */
+	if (valObj == opts->formatObj) {
+	    /* mark it as unlocalizable, by setting self as key (without refcount incr) */
+	    if (opts->formatObj->typePtr == &ClockFmtObjType) {
+		Tcl_UnsetObjRef(ObjLocFmtKey(opts->formatObj));
+		ObjLocFmtKey(opts->formatObj) = opts->formatObj;
+	    }
+	}
+clean:
+
+	Tcl_UnsetObjRef(keyObj);
+	Tcl_ResetResult(opts->interp);
+    }
+
+    return (opts->formatObj = valObj);
+}
+
 /*
  *----------------------------------------------------------------------
  */
@@ -917,7 +987,6 @@ ClockScan(
     int ret = TCL_ERROR;
 
     /* get localized format */
-
     if (ClockLocalizeFormat(opts) == NULL) {
 	return TCL_ERROR;
     }
diff --git a/generic/tclDate.h b/generic/tclDate.h
index 62fa693..9f65b1b 100644
--- a/generic/tclDate.h
+++ b/generic/tclDate.h
@@ -37,6 +37,53 @@
 
 
 /*
+ * Enumeration of the string literals used in [clock]
+ */
+
+typedef enum ClockLiteral {
+    LIT__NIL,
+    LIT__DEFAULT_FORMAT,
+    LIT_BCE,		LIT_C,
+    LIT_CANNOT_USE_GMT_AND_TIMEZONE,
+    LIT_CE,
+    LIT_DAYOFMONTH,	LIT_DAYOFWEEK,		LIT_DAYOFYEAR,
+    LIT_ERA,		LIT_GMT,		LIT_GREGORIAN,
+    LIT_INTEGER_VALUE_TOO_LARGE,
+    LIT_ISO8601WEEK,	LIT_ISO8601YEAR,
+    LIT_JULIANDAY,	LIT_LOCALSECONDS,
+    LIT_MONTH,
+    LIT_SECONDS,	LIT_TZNAME,		LIT_TZOFFSET,
+    LIT_YEAR,
+    LIT_TZDATA,
+    LIT_GETSYSTEMTIMEZONE,
+    LIT_SETUPTIMEZONE,
+    LIT_MCGET,		LIT_TCL_CLOCK,
+    LIT_LOCALIZE_FORMAT,
+    LIT__END
+} ClockLiteral;
+
+#define CLOCK_LITERAL_ARRAY(litarr) static const char *const litarr[] = { \
+    "", \
+    "%a %b %d %H:%M:%S %Z %Y", \
+    "BCE",		"C", \
+    "cannot use -gmt and -timezone in same call", \
+    "CE", \
+    "dayOfMonth",	"dayOfWeek",		"dayOfYear", \
+    "era",		":GMT",			"gregorian", \
+    "integer value too large to represent", \
+    "iso8601Week",	"iso8601Year", \
+    "julianDay",	"localSeconds", \
+    "month", \
+    "seconds",		"tzName",		"tzOffset", \
+    "year", \
+    "::tcl::clock::TZData", \
+    "::tcl::clock::GetSystemTimeZone", \
+    "::tcl::clock::SetupTimeZone", \
+    "::msgcat::mcget", "::tcl::clock", \
+    "::tcl::clock::LocalizeFormat" \
+}
+
+/*
  * Enumeration of the msgcat literals used in [clock]
  */
 
@@ -362,8 +409,6 @@ MODULE_SCOPE Tcl_Obj *
 		    ClockMCGet(ClockFmtScnCmdArgs *opts, int mcKey);
 MODULE_SCOPE Tcl_Obj *
 		    ClockMCGetListIdxDict(ClockFmtScnCmdArgs *opts, int mcKey);
-MODULE_SCOPE Tcl_Obj *
-		    ClockLocalizeFormat(ClockFmtScnCmdArgs *opts);
 
 /* tclClockFmt.c module declarations */
 
@@ -374,7 +419,8 @@ MODULE_SCOPE Tcl_Obj*
 MODULE_SCOPE ClockFmtScnStorage * 
 		    Tcl_GetClockFrmScnFromObj(Tcl_Interp *interp,
 			Tcl_Obj *objPtr);
-
+MODULE_SCOPE Tcl_Obj *
+		    ClockLocalizeFormat(ClockFmtScnCmdArgs *opts);
 MODULE_SCOPE int    ClockScan(ClientData clientData, Tcl_Interp *interp,
 			register DateInfo *info,
 			Tcl_Obj *strObj, ClockFmtScnCmdArgs *opts);
diff --git a/library/clock.tcl b/library/clock.tcl
index 4173174..a532c0d 100755
--- a/library/clock.tcl
+++ b/library/clock.tcl
@@ -2365,18 +2365,23 @@ proc ::tcl::clock::LocalizeFormat { locale format {fmtkey {}} } {
 	    dict set LocaleFormats $locale MLST $mlst
 	}
 
-	# translate:
-	set locfmt [string map $mlst $format]
-
-	# Save original format as long as possible, because of internal representation (performance)
-	if {$locfmt eq $format} {
-	    set locfmt $format
-	}
+	# translate copy of format (don't use format object here, because otherwise 
+	# it can lose its internal representation (string map - convert to unicode)
+	set locfmt [string map $mlst [string range " $format" 1 end]]
 
 	# cache it:
 	dict set LocaleFormats $locale $fmtkey $locfmt
     }
 
+    # Save original format as long as possible, because of internal 
+    # representation (performance).
+    # Note that in this case such format will be never localized (also
+    # using another locales). To prevent this return a duplicate (but 
+    # it may be slower).
+    if {$locfmt eq $format} {
+        set locfmt $format
+    }
+
     return $locfmt
 }
 
diff --git a/tests-perf/clock.perf.tcl b/tests-perf/clock.perf.tcl
index a005648..3c69414 100644
--- a/tests-perf/clock.perf.tcl
+++ b/tests-perf/clock.perf.tcl
@@ -132,9 +132,6 @@ proc test-scan {{reptime 1000}} {
     # Scan : date-time (system time zone without base)
     {clock scan "25.11.2015 10:35:55" -format "%d.%m.%Y %H:%M:%S"}
 
-    # Scan : dynamic format (cacheable)
-    {clock scan "25.11.2015 10:35:55" -format [string trim "%d.%m.%Y %H:%M:%S "] -base 0 -gmt 1}
-
     # Scan : julian day in gmt
     {clock scan 2451545 -format %J -gmt 1}
     # Scan : julian day in system TZ
@@ -153,6 +150,9 @@ proc test-scan {{reptime 1000}} {
     # Scan : century, lookup table month and day (list scan: entries with position 12 / 31)
     {clock scan {2016 Dec 31} -format {%C%y %b %Od} -locale en -gmt 1}
 
+    # Scan : dynamic format (cacheable)
+    {clock scan "25.11.2015 10:35:55" -format [string trim "%d.%m.%Y %H:%M:%S "] -base 0 -gmt 1}
+
     break
 
     # Scan : zone only