ICU-23000 Replace CharString for LocaleBased

The changes improve the overall peak memory usage and slightly improve performance main avg main stdev branch avg branch stdev diff avg diff avg % User time (seconds): 81.19 0.37 81.36 0.09 0.17 0.21% System time (seconds): 5.03 0.08 4.86 0.05 -0.17 -3.38% Percent of CPU this job got: 237.33% 0.01 235.33% 0.01 -2.00% -0.84% Maximum resident set size (kbytes): 227,141 99.14 226,667 366.48 -475 -0.21% Minor (reclaiming a frame) page faults: 46,009 510.78 43,775 294.47 -2,234 -4.86% Voluntary context switches: 333,442 4513.76 316,506 8850.48 -16,936 -5.08% Involuntary context switches: 467 30.60 515 22.65 48 10.36% Raw data in https://docs.google.com/spreadsheets/d/10LQ3kO83EwvsFfoiySTzsTbIzfUV00Te4jQnX0b_FM4/edit?usp=sharing
unicode-org · Dec 26, 2024 · 8554c0a · 8554c0a
1 parent e515c84
commit 8554c0a
Show file tree

Hide file tree

Showing 12 changed files with 137 additions and 90 deletions.
diff --git a/icu4c/source/common/brkiter.cpp b/icu4c/source/common/brkiter.cpp
@@ -123,10 +123,16 @@ BreakIterator::buildInstance(const Locale& loc, const char *type, UErrorCode &st
     if (U_SUCCESS(status) && result != nullptr) {
         U_LOCALE_BASED(locBased, *(BreakIterator*)result);
 
-        locBased.setLocaleIDs(ures_getLocaleByType(b, ULOC_VALID_LOCALE, &status), 
-                              actualLocale.data());
-        uprv_strncpy(result->requestLocale, loc.getName(), ULOC_FULLNAME_CAPACITY);
-        result->requestLocale[ULOC_FULLNAME_CAPACITY-1] = 0; // always terminate
+        locBased.setLocaleIDs(ures_getLocaleByType(b, ULOC_VALID_LOCALE, &status),
+                              actualLocale.data(), status);
+        if (U_FAILURE(status)) {
+            return nullptr;
+        }
+        result->requestLocale->clear();
+        result->requestLocale->append(loc.getName(), -1, status);
+        if (U_FAILURE(status)) {
+            return nullptr;
+        }
     }
 
     ures_close(b);
@@ -204,28 +210,34 @@ BreakIterator::getAvailableLocales(int32_t& count)
 //
 //-------------------------------------------
 
-BreakIterator::BreakIterator()
+BreakIterator::BreakIterator() : actualLocale(new CharString()), validLocale(new CharString), requestLocale(new CharString())
 {
-    *validLocale = *actualLocale = *requestLocale = 0;
 }
 
-BreakIterator::BreakIterator(const BreakIterator &other) : UObject(other) {
-    uprv_strncpy(actualLocale, other.actualLocale, sizeof(actualLocale));
-    uprv_strncpy(validLocale, other.validLocale, sizeof(validLocale));
-    uprv_strncpy(requestLocale, other.requestLocale, sizeof(requestLocale));
+BreakIterator::BreakIterator(const BreakIterator &other) : UObject(other), actualLocale(new CharString()), validLocale(new CharString), requestLocale(new CharString()) {
+    UErrorCode status = U_ZERO_ERROR;
+    actualLocale->copyFrom(*(other.actualLocale), status);
+    validLocale->copyFrom(*(other.validLocale), status);
+    requestLocale->copyFrom(*(other.requestLocale), status);
+    U_ASSERT(U_SUCCESS(status));
 }
 
 BreakIterator &BreakIterator::operator =(const BreakIterator &other) {
     if (this != &other) {
-        uprv_strncpy(actualLocale, other.actualLocale, sizeof(actualLocale));
-        uprv_strncpy(validLocale, other.validLocale, sizeof(validLocale));
-        uprv_strncpy(requestLocale, other.requestLocale, sizeof(requestLocale));
+        UErrorCode status = U_ZERO_ERROR;
+        actualLocale->copyFrom(*(other.actualLocale), status);
+        validLocale->copyFrom(*(other.validLocale), status);
+        requestLocale->copyFrom(*(other.requestLocale), status);
+        U_ASSERT(U_SUCCESS(status));
     }
     return *this;
 }
 
 BreakIterator::~BreakIterator()
 {
+    delete validLocale;
+    delete actualLocale;
+    delete requestLocale;
 }
 
 // ------------------------------------------
@@ -394,7 +406,7 @@ BreakIterator::createInstance(const Locale& loc, int32_t kind, UErrorCode& statu
         // revisit this in ICU 3.0 and clean it up/fix it/remove it.
         if (U_SUCCESS(status) && (result != nullptr) && *actualLoc.getName() != 0) {
             U_LOCALE_BASED(locBased, *result);
-            locBased.setLocaleIDs(actualLoc.getName(), actualLoc.getName());
+            locBased.setLocaleIDs(actualLoc.getName(), actualLoc.getName(), status);
         }
         return result;
     }
@@ -497,7 +509,7 @@ BreakIterator::makeInstance(const Locale& loc, int32_t kind, UErrorCode& status)
 Locale
 BreakIterator::getLocale(ULocDataLocaleType type, UErrorCode& status) const {
     if (type == ULOC_REQUESTED_LOCALE) {
-        return {requestLocale};
+        return {requestLocale->data()};
     }
     U_LOCALE_BASED(locBased, *this);
     return locBased.getLocale(type, status);
@@ -506,7 +518,7 @@ BreakIterator::getLocale(ULocDataLocaleType type, UErrorCode& status) const {
 const char *
 BreakIterator::getLocaleID(ULocDataLocaleType type, UErrorCode& status) const {
     if (type == ULOC_REQUESTED_LOCALE) {
-        return requestLocale;
+        return requestLocale->data();
     }
     U_LOCALE_BASED(locBased, *this);
     return locBased.getLocaleID(type, status);
@@ -535,9 +547,11 @@ int32_t BreakIterator::getRuleStatusVec(int32_t *fillInVec, int32_t capacity, UE
     return 1;
 }
 
-BreakIterator::BreakIterator (const Locale& valid, const Locale& actual) {
+BreakIterator::BreakIterator (const Locale& valid, const Locale& actual) : actualLocale(new CharString()), validLocale(new CharString), requestLocale(new CharString()) {
   U_LOCALE_BASED(locBased, (*this));
-  locBased.setLocaleIDs(valid, actual);
+  UErrorCode status = U_ZERO_ERROR;
+  locBased.setLocaleIDs(valid, actual, status);
+  U_ASSERT(U_SUCCESS(status));
 }
 
 U_NAMESPACE_END

diff --git a/icu4c/source/common/locbased.cpp b/icu4c/source/common/locbased.cpp
@@ -12,6 +12,7 @@
 */
 #include "locbased.h"
 #include "cstring.h"
+#include "charstr.h"
 
 U_NAMESPACE_BEGIN
 
@@ -27,29 +28,38 @@ const char* LocaleBased::getLocaleID(ULocDataLocaleType type, UErrorCode& status
 
     switch(type) {
     case ULOC_VALID_LOCALE:
-        return valid;
+        return valid->data();
     case ULOC_ACTUAL_LOCALE:
-        return actual;
+        return actual->data();
     default:
         status = U_ILLEGAL_ARGUMENT_ERROR;
         return nullptr;
     }
 }
 
-void LocaleBased::setLocaleIDs(const char* validID, const char* actualID) {
+void LocaleBased::setLocaleIDs(const char* validID, const char* actualID, UErrorCode& status) {
+    if (U_FAILURE(status)) {
+        return;
+    }
     if (validID != nullptr) {
-      uprv_strncpy(valid, validID, ULOC_FULLNAME_CAPACITY);
-      valid[ULOC_FULLNAME_CAPACITY-1] = 0; // always terminate
+      valid->clear();
+      valid->append(validID, -1, status);
     }
     if (actualID != nullptr) {
-      uprv_strncpy(actual, actualID, ULOC_FULLNAME_CAPACITY);
-      actual[ULOC_FULLNAME_CAPACITY-1] = 0; // always terminate
+      actual->clear();
+      actual->append(actualID, -1, status);
     }
 }
 
-void LocaleBased::setLocaleIDs(const Locale& validID, const Locale& actualID) {
-  uprv_strcpy(valid, validID.getName());
-  uprv_strcpy(actual, actualID.getName());
+void LocaleBased::setLocaleIDs(const Locale& validID, const Locale& actualID, UErrorCode& status) {
+    if (U_FAILURE(status)) {
+        return;
+    }
+    valid->clear();
+    valid->append(validID.getName(), -1, status);
+    actual->clear();
+    actual->append(actualID.getName(), -1, status);
+    U_ASSERT(U_SUCCESS(status));
 }
 
 U_NAMESPACE_END
diff --git a/icu4c/source/common/locbased.h b/icu4c/source/common/locbased.h
@@ -19,13 +19,14 @@
 /**
  * Macro to declare a locale LocaleBased wrapper object for the given
  * object, which must have two members named `validLocale' and
- * `actualLocale' of size ULOC_FULLNAME_CAPACITY
+ * `actualLocale' of CharString.
  */
 #define U_LOCALE_BASED(varname, objname) \
   LocaleBased varname((objname).validLocale, (objname).actualLocale)
 
 U_NAMESPACE_BEGIN
 
+class CharString;
 /**
  * A utility class that unifies the implementation of getLocale() by
  * various ICU services.  This class is likely to be removed in the
@@ -41,13 +42,13 @@ class U_COMMON_API LocaleBased : public UMemory {
      * Construct a LocaleBased wrapper around the two pointers.  These
      * will be aliased for the lifetime of this object.
      */
-    inline LocaleBased(char* validAlias, char* actualAlias);
+    inline LocaleBased(CharString* validAlias, CharString* actualAlias);
 
     /**
      * Construct a LocaleBased wrapper around the two const pointers.
      * These will be aliased for the lifetime of this object.
      */
-    inline LocaleBased(const char* validAlias, const char* actualAlias);
+    inline LocaleBased(const CharString* validAlias, const CharString* actualAlias);
 
     /**
      * Return locale meta-data for the service object wrapped by this
@@ -75,31 +76,31 @@ class U_COMMON_API LocaleBased : public UMemory {
      * @param valid the ID of the valid locale
      * @param actual the ID of the actual locale
      */
-    void setLocaleIDs(const char* valid, const char* actual);
+    void setLocaleIDs(const char* valid, const char* actual, UErrorCode& status);
 
     /**
      * Set the locale meta-data for the service object wrapped by this
      * object.
      * @param valid the ID of the valid locale
      * @param actual the ID of the actual locale
      */
-    void setLocaleIDs(const Locale& valid, const Locale& actual);
+    void setLocaleIDs(const Locale& valid, const Locale& actual, UErrorCode& status);
 
  private:
 
-    char* valid;
+    CharString* valid;
 
-    char* actual;
+    CharString* actual;
 };
 
-inline LocaleBased::LocaleBased(char* validAlias, char* actualAlias) :
+inline LocaleBased::LocaleBased(CharString* validAlias, CharString* actualAlias) :
     valid(validAlias), actual(actualAlias) {
 }
 
-inline LocaleBased::LocaleBased(const char* validAlias,
-                                const char* actualAlias) :
+inline LocaleBased::LocaleBased(const CharString* validAlias,
+                                const CharString* actualAlias) :
     // ugh: cast away const
-    valid(const_cast<char*>(validAlias)), actual(const_cast<char*>(actualAlias)) {
+    valid(const_cast<CharString*>(validAlias)), actual(const_cast<CharString*>(actualAlias)) {
 }
 
 U_NAMESPACE_END

diff --git a/icu4c/source/common/unicode/brkiter.h b/icu4c/source/common/unicode/brkiter.h
@@ -58,6 +58,8 @@ U_NAMESPACE_END
 
 U_NAMESPACE_BEGIN
 
+class CharString;
+
 /**
  * The BreakIterator class implements methods for finding the location
  * of boundaries in text. BreakIterator is an abstract base class.
@@ -646,9 +648,9 @@ class U_COMMON_API BreakIterator : public UObject {
 private:
 
     /** @internal (private) */
-    char actualLocale[ULOC_FULLNAME_CAPACITY];
-    char validLocale[ULOC_FULLNAME_CAPACITY];
-    char requestLocale[ULOC_FULLNAME_CAPACITY];
+    CharString* actualLocale;
+    CharString* validLocale;
+    CharString* requestLocale;
 };
 
 #ifndef U_HIDE_DEPRECATED_API

diff --git a/icu4c/source/i18n/calendar.cpp b/icu4c/source/i18n/calendar.cpp
@@ -705,10 +705,10 @@ fTime(0),
 fLenient(true),
 fZone(nullptr),
 fRepeatedWallTime(UCAL_WALLTIME_LAST),
-fSkippedWallTime(UCAL_WALLTIME_LAST)
+fSkippedWallTime(UCAL_WALLTIME_LAST),
+validLocale(new CharString()),
+actualLocale(new CharString())
 {
-    validLocale[0] = 0;
-    actualLocale[0] = 0;
     clear();
     if (U_FAILURE(success)) {
         return;
@@ -733,10 +733,10 @@ fTime(0),
 fLenient(true),
 fZone(nullptr),
 fRepeatedWallTime(UCAL_WALLTIME_LAST),
-fSkippedWallTime(UCAL_WALLTIME_LAST)
+fSkippedWallTime(UCAL_WALLTIME_LAST),
+validLocale(new CharString()),
+actualLocale(new CharString())
 {
-    validLocale[0] = 0;
-    actualLocale[0] = 0;
     if (U_FAILURE(success)) {
         delete zone;
         return;
@@ -768,10 +768,10 @@ fTime(0),
 fLenient(true),
 fZone(nullptr),
 fRepeatedWallTime(UCAL_WALLTIME_LAST),
-fSkippedWallTime(UCAL_WALLTIME_LAST)
+fSkippedWallTime(UCAL_WALLTIME_LAST),
+validLocale(new CharString()),
+actualLocale(new CharString())
 {
-    validLocale[0] = 0;
-    actualLocale[0] = 0;
     if (U_FAILURE(success)) {
         return;
     }
@@ -788,12 +788,14 @@ fSkippedWallTime(UCAL_WALLTIME_LAST)
 Calendar::~Calendar()
 {
     delete fZone;
+    delete actualLocale;
+    delete validLocale;
 }
 
 // -------------------------------------
 
 Calendar::Calendar(const Calendar &source)
-:   UObject(source)
+:   UObject(source), validLocale(new CharString()), actualLocale(new CharString())
 {
     fZone = nullptr;
     *this = source;
@@ -828,10 +830,10 @@ Calendar::operator=(const Calendar &right)
         fWeekendCease            = right.fWeekendCease;
         fWeekendCeaseMillis      = right.fWeekendCeaseMillis;
         fNextStamp               = right.fNextStamp;
-        uprv_strncpy(validLocale, right.validLocale, sizeof(validLocale)-1);
-        validLocale[sizeof(validLocale)-1] = 0;
-        uprv_strncpy(actualLocale, right.actualLocale, sizeof(actualLocale)-1);
-        actualLocale[sizeof(validLocale)-1] = 0;
+        UErrorCode status = U_ZERO_ERROR;
+        validLocale->copyFrom(*right.validLocale, status);
+        actualLocale->copyFrom(*right.actualLocale, status);
+        U_ASSERT(U_SUCCESS(status));
     }
 
     return *this;
@@ -4137,7 +4139,7 @@ Calendar::setWeekData(const Locale& desiredLocale, const char *type, UErrorCode&
     if (U_SUCCESS(status)) {
         U_LOCALE_BASED(locBased,*this);
         locBased.setLocaleIDs(ures_getLocaleByType(monthNames.getAlias(), ULOC_VALID_LOCALE, &status),
-                              ures_getLocaleByType(monthNames.getAlias(), ULOC_ACTUAL_LOCALE, &status));
+                              ures_getLocaleByType(monthNames.getAlias(), ULOC_ACTUAL_LOCALE, &status), status);
     } else {
         status = U_USING_FALLBACK_WARNING;
         return;