unicode-org · sffc · Jul 9, 2019 · Jun 18, 2019
diff --git a/docs/userguide/icu_data/tracing.md b/docs/userguide/icu_data/tracing.md
@@ -60,17 +60,20 @@ int main() {
 
 The following output is produced from this program:
 
-    FileTracer::traceOpenResFile icudt64l-brkitr/zh_CN.res
-    FileTracer::traceOpenResFile icudt64l-brkitr/zh.res
-    FileTracer::traceOpenResFile icudt64l-brkitr/root.res
-    ResourceTracer::trace (string) icudt64l-brkitr/root.res @ /boundaries/word
-    FileTracer::traceOpenDataFile icudt64l-brkitr/word.brk
+    res-open icudt64l-brkitr/zh_CN.res
+    res-open icudt64l-brkitr/zh.res
+    res-open icudt64l-brkitr/root.res
+    bundle-open icudt64l-brkitr/zh.res
+    resc       (get) icudt64l-brkitr/zh.res @ /boundaries
+    resc       (get) icudt64l-brkitr/root.res @ /boundaries/word
+    resc    (string) icudt64l-brkitr/root.res @ /boundaries/word
+    file-open icudt64l-brkitr/word.brk
 
 What this means:
 
 1. The BreakIterator constructor opened three resource files in the locale
-   fallback chain for zh_CN.
-2. One string was read from the resource bundle: the one at the resource path
+   fallback chain for zh_CN. The actual bundle was opened for zh.
+2. One string was read from that resource bundle: the one at the resource path
    "/boundaries/word" in brkitr/root.res.
 3. In addition, the binary data file brkitr/word.brk was opened.
 
@@ -97,7 +100,7 @@ bundle was read by ICU code.
 When `fnNumber` is `UTRACE_UDATA_RESOURCE`, there are three C-style strings in
 `args`:
 
-1. Data type; not relevant for the purpose of resource filtering.
+1. Data type; not usually relevant for the purpose of resource filtering.
 2. The internal path of the resource file from which the value was read.
 3. The path to the value within that resource file.
 
@@ -112,21 +115,26 @@ const char* resPath = va_arg(args, const char*);
 As stated above, you should copy the strings if you intend to save them. The
 pointers will not be valid after the tracing function returns.
 
+### UTRACE_UDATA_BUNDLE
+
+UTRACE_UDATA_BUNDLE is used to indicate that a resource bundle was opened by
+ICU code.
+
+For the purposes of making your ICU data filter, the specific resource paths
+provided by UTRACE_UDATA_RESOURCE are more precise and useful.
+
 ### UTRACE_UDATA_DATA_FILE
 
 UTRACE_UDATA_DATA_FILE is used to indicate that a non-resource-bundle binary
 data file was opened by ICU code. Such files are used for break iteration,
 conversion, confusables, and a handful of other ICU services.
 
-When `fnNumber` is `UTRACE_UDATA_DATA_FILE`, there is just one C string in
-`args`: the internal path of the data file that was opened.
-
 ### UTRACE_UDATA_RES_FILE
 
 UTRACE_UDATA_RES_FILE is used to indicate that a binary resource bundle file
-was opened by ICU code. This can be helpful to debug locale fallbacks. For the
-purposes of making your ICU data filter, the specific resource paths provided
-by UTRACE_UDATA_RESOURCE are more precise and useful.
+was opened by ICU code. This can be helpful to debug locale fallbacks. This
+differs from UTRACE_UDATA_BUNDLE because the resource *file* is typically
+opened only once per application runtime.
 
-When `fnNumber` is `UTRACE_UDATA_RES_FILE`, there is just one C string in
-`args`: the internal path of the resource file that was opened.
+For the purposes of making your ICU data filter, the specific resource paths
+provided by UTRACE_UDATA_RESOURCE are more precise and useful.
diff --git a/icu4c/source/common/restrace.cpp b/icu4c/source/common/restrace.cpp
@@ -22,20 +22,37 @@ void ResourceTracer::trace(const char* resType) const {
     UTRACE_ENTRY(UTRACE_UDATA_RESOURCE);
     UErrorCode status = U_ZERO_ERROR;
 
-    icu::CharString filePath;
+    CharString filePath;
     getFilePath(filePath, status);
 
-    icu::CharString resPath;
+    CharString resPath;
     getResPath(resPath, status);
 
-    UTRACE_DATA3(UTRACE_VERBOSE, "(%s) %s @ %s",
+    // The longest type ("intvector") is 9 chars
+    const char kSpaces[] = "         ";
+    CharString format;
+    format.append(kSpaces, sizeof(kSpaces) - 1 - uprv_strlen(resType), status);
+    format.append("(%s) %s @ %s", status);
+
+    UTRACE_DATA3(UTRACE_VERBOSE,
+        format.data(),
         resType,
         filePath.data(),
         resPath.data());
     UTRACE_EXIT_STATUS(status);
 }
 
-void ResourceTracer::getFilePath(CharString& output, UErrorCode& status) const {
+void ResourceTracer::traceOpen() const {
+    U_ASSERT(fResB);
+    UTRACE_ENTRY(UTRACE_UDATA_BUNDLE);
+    UErrorCode status = U_ZERO_ERROR;
+
+    CharString filePath;
+    UTRACE_DATA1(UTRACE_VERBOSE, "%s", getFilePath(filePath, status).data());
+    UTRACE_EXIT_STATUS(status);
+}
+
+CharString& ResourceTracer::getFilePath(CharString& output, UErrorCode& status) const {
     if (fResB) {
         output.append(fResB->fData->fPath, status);
         output.append('/', status);
@@ -44,9 +61,10 @@ void ResourceTracer::getFilePath(CharString& output, UErrorCode& status) const {
     } else {
         fParent->getFilePath(output, status);
     }
+    return output;
 }
 
-void ResourceTracer::getResPath(CharString& output, UErrorCode& status) const {
+CharString& ResourceTracer::getResPath(CharString& output, UErrorCode& status) const {
     if (fResB) {
         output.append('/', status);
         output.append(fResB->fResPath, status);
@@ -67,6 +85,7 @@ void ResourceTracer::getResPath(CharString& output, UErrorCode& status) const {
         output.appendInvariantChars(indexString, status);
         output.append(']', status);
     }
+    return output;
 }
 
 void FileTracer::traceOpen(const char* path, const char* type, const char* name) {
@@ -81,7 +100,7 @@ void FileTracer::traceOpenDataFile(const char* path, const char* type, const cha
     UTRACE_ENTRY(UTRACE_UDATA_DATA_FILE);
     UErrorCode status = U_ZERO_ERROR;
 
-    icu::CharString filePath;
+    CharString filePath;
     filePath.append(path, status);
     filePath.append('/', status);
     filePath.append(name, status);
@@ -96,7 +115,7 @@ void FileTracer::traceOpenResFile(const char* path, const char* name) {
     UTRACE_ENTRY(UTRACE_UDATA_RES_FILE);
     UErrorCode status = U_ZERO_ERROR;
 
-    icu::CharString filePath;
+    CharString filePath;
     filePath.append(path, status);
     filePath.append('/', status);
     filePath.append(name, status);

diff --git a/icu4c/source/common/restrace.h b/icu4c/source/common/restrace.h
@@ -66,16 +66,27 @@ class U_COMMON_API ResourceTracer {
     ~ResourceTracer();
 
     void trace(const char* type) const;
+    void traceOpen() const;
+
+    /**
+     * Calls trace() if the resB or parent provided to the constructor was
+     * non-null; otherwise, does nothing.
+     */
+    void maybeTrace(const char* type) const {
+        if (fResB || fParent) {
+            trace(type);
+        }
+    }
 
 private:
     const UResourceBundle* fResB;
     const ResourceTracer* fParent;
     const char* fKey;
     int32_t fIndex;
 
-    void getFilePath(CharString& output, UErrorCode& status) const;
+    CharString& getFilePath(CharString& output, UErrorCode& status) const;
 
-    void getResPath(CharString& output, UErrorCode& status) const;
+    CharString& getResPath(CharString& output, UErrorCode& status) const;
 };
 
 /**
@@ -115,6 +126,10 @@ class U_COMMON_API ResourceTracer {
     ResourceTracer(const ResourceTracer&, int32_t) {}
 
     void trace(const char*) const {}
+
+    void traceOpen() const {}
+
+    void maybeTrace(const char*) const {}
 };
 
 /**

diff --git a/icu4c/source/common/unicode/utrace.h b/icu4c/source/common/unicode/utrace.h
@@ -118,26 +118,50 @@ typedef enum UTraceFunctionNumber {
      * The lowest resource/data location.
      * @draft ICU 65
      */
-    UTRACE_RES_DATA_START=0x3000,
+    UTRACE_UDATA_START=0x3000,
 
     /**
      * Indicates that a value was read from a resource bundle. Provides three
      * C-style strings to UTraceData: type, file name, and resource path. The
-     * type is "string", "binary", "intvector", "int", or "uint".
+     * possible types are:
+     *
+     * - "string" (a string value was accessed)
+     * - "binary" (a binary value was accessed)
+     * - "intvector" (a integer vector value was accessed)
+     * - "int" (a signed integer value was accessed)
+     * - "uint" (a unsigned integer value was accessed)
+     * - "get" (a path was loaded, but the value was not accessed)
+     * - "getalias" (a path was loaded, and an alias was resolved)
+     *
      * @draft ICU 65
      */
-    UTRACE_UDATA_RESOURCE=UTRACE_RES_DATA_START,
+    UTRACE_UDATA_RESOURCE=UTRACE_UDATA_START,
 
     /**
-     * Indicates that a value was read from a resource bundle. Provides one
-     * C-style string to UTraceData: file name.
+     * Indicates that a resource bundle was opened.
+     *
+     * Provides one C-style string to UTraceData: file name.
+     */
+    UTRACE_UDATA_BUNDLE,
+
+    /**
+     * Indicates that a data file was opened, but not *.res files.
+     *
+     * Provides one C-style string to UTraceData: file name.
+     *
      * @draft ICU 65
      */
     UTRACE_UDATA_DATA_FILE,
 
     /**
-     * Indicates that a value was read from a resource bundle. Provides one
-     * C-style string to UTraceData: file name.
+     * Indicates that a *.res file was opened.
+     *
+     * This differs from UTRACE_UDATA_BUNDLE because a res file is typically
+     * opened only once per application runtime, but the bundle corresponding
+     * to that res file may be opened many times.
+     *
+     * Provides one C-style string to UTraceData: file name.
+     *
      * @draft ICU 65
      */
     UTRACE_UDATA_RES_FILE,

diff --git a/icu4c/source/common/uresbund.cpp b/icu4c/source/common/uresbund.cpp
@@ -1166,6 +1166,7 @@ static UResourceBundle *init_resb_result(const ResourceData *rdata, Resource r,
                     if(mainRes != result) {
                         ures_close(mainRes);
                     }
+                    ResourceTracer(resB).maybeTrace("getalias");
                     return result;
                 }
             } else {
@@ -1245,6 +1246,7 @@ static UResourceBundle *init_resb_result(const ResourceData *rdata, Resource r,
     /*resB->fParent = parent->fRes;*/
     uprv_memmove(&resB->fResData, rdata, sizeof(ResourceData));
     resB->fSize = res_countArrayItems(&(resB->fResData), resB->fRes);
+    ResourceTracer(resB).trace("get");
     return resB;
 }
 
@@ -2294,6 +2296,8 @@ ures_openWithType(UResourceBundle *r, const char* path, const char* localeID,
     r->fSize = res_countArrayItems(&(r->fResData), r->fRes);
     r->fIndex = -1;
 
+    ResourceTracer(r).traceOpen();
+
     return r;
 }
 

diff --git a/icu4c/source/common/utrace.cpp b/icu4c/source/common/utrace.cpp
@@ -479,9 +479,10 @@ trCollNames[] = {
 
 static const char* const
 trResDataNames[] = {
-    "ResourceTracer::trace",
-    "FileTracer::traceOpenDataFile",
-    "FileTracer::traceOpenResFile",
+    "resc",
+    "bundle-open",
+    "file-open",
+    "res-open",
     NULL
 };
 
@@ -494,8 +495,8 @@ utrace_functionName(int32_t fnNumber) {
         return trConvNames[fnNumber - UTRACE_CONVERSION_START];
     } else if(UTRACE_COLLATION_START <= fnNumber && fnNumber < UTRACE_COLLATION_LIMIT){
         return trCollNames[fnNumber - UTRACE_COLLATION_START];
-    } else if(UTRACE_RES_DATA_START <= fnNumber && fnNumber < UTRACE_RES_DATA_LIMIT){
-        return trResDataNames[fnNumber - UTRACE_RES_DATA_START];
+    } else if(UTRACE_UDATA_START <= fnNumber && fnNumber < UTRACE_RES_DATA_LIMIT){
+        return trResDataNames[fnNumber - UTRACE_UDATA_START];
     } else {
         return "[BOGUS Trace Function Number]";
     }

diff --git a/icu4c/source/test/intltest/restsnew.cpp b/icu4c/source/test/intltest/restsnew.cpp
@@ -1356,7 +1356,7 @@ void NewResourceBundleTest::TestFilter() {
 
 static std::vector<std::string> gResourcePathsTraced;
 static std::vector<std::string> gDataFilesTraced;
-static std::vector<std::string> gResFilesTraced;
+static std::vector<std::string> gBundlesTraced;
 
 static void U_CALLCONV traceData(
         const void*,
@@ -1365,23 +1365,30 @@ static void U_CALLCONV traceData(
         const char *,
         va_list args) {
 
+    // NOTE: Whether this test is run in isolation affects whether or not
+    // *.res files are opened. For stability, ignore *.res file opens.
+
     if (fnNumber == UTRACE_UDATA_RESOURCE) {
         va_arg(args, const char*); // type
         va_arg(args, const char*); // file
         const char* resourcePath = va_arg(args, const char*);
         gResourcePathsTraced.push_back(resourcePath);
+    } else if (fnNumber == UTRACE_UDATA_BUNDLE) {
+        const char* filePath = va_arg(args, const char*);
+        gBundlesTraced.push_back(filePath);
     } else if (fnNumber == UTRACE_UDATA_DATA_FILE) {
         const char* filePath = va_arg(args, const char*);
         gDataFilesTraced.push_back(filePath);
     } else if (fnNumber == UTRACE_UDATA_RES_FILE) {
-        const char* filePath = va_arg(args, const char*);
-        gResFilesTraced.push_back(filePath);
+        // ignore
     }
 }
 
 void NewResourceBundleTest::TestTrace() {
     IcuTestErrorCode status(*this, "TestTrace");
 
+    assertEquals("Start position stability coverage", 0x3000, UTRACE_UDATA_START);
+
     const void* context;
     utrace_setFunctions(context, nullptr, nullptr, traceData);
     utrace_setLevel(UTRACE_VERBOSE);
@@ -1390,42 +1397,34 @@ void NewResourceBundleTest::TestTrace() {
         LocalPointer<BreakIterator> brkitr(BreakIterator::createWordInstance("zh-CN", status));
 
         assertEquals("Should touch expected resource paths",
-            { "/boundaries/word" },
+            { "/boundaries", "/boundaries/word", "/boundaries/word" },
             gResourcePathsTraced);
+        assertEquals("Should touch expected resource bundles",
+            { U_ICUDATA_NAME "-brkitr/zh.res" },
+            gBundlesTraced);
         assertEquals("Should touch expected data files",
             { U_ICUDATA_NAME "-brkitr/word.brk" },
             gDataFilesTraced);
-        // NOTE: The following passes only when this test is run in isolation.
-        // If run in "make check", these files were already open.
-        // assertEquals("Should touch expected resource files",
-        //     {
-        //         U_ICUDATA_NAME "-brkitr/zh_CN.res",
-        //         U_ICUDATA_NAME "-brkitr/zh.res",
-        //         U_ICUDATA_NAME "-brkitr/root.res"
-        //     },
-        //     gResFilesTraced);
         gResourcePathsTraced.clear();
         gDataFilesTraced.clear();
-        gResFilesTraced.clear();
+        gBundlesTraced.clear();
     }
 
     {
         ucurr_getDefaultFractionDigits(u"USD", status);
 
         assertEquals("Should touch expected resource paths",
-            { "/CurrencyMeta/DEFAULT" },
+            { "/CurrencyMeta", "/CurrencyMeta/DEFAULT", "/CurrencyMeta/DEFAULT" },
             gResourcePathsTraced);
+        assertEquals("Should touch expected resource bundles",
+            { U_ICUDATA_NAME "-curr/supplementalData.res" },
+            gBundlesTraced);
         assertEquals("Should touch expected data files",
             { },
             gDataFilesTraced);
-        // NOTE: The following passes only when this test is run in isolation.
-        // If run in "make check", these files were already open.
-        // assertEquals("Should touch expected resource files",
-        //     { U_ICUDATA_NAME "-curr/supplementalData.res" },
-        //     gResFilesTraced);
         gResourcePathsTraced.clear();
         gDataFilesTraced.clear();
-        gResFilesTraced.clear();
+        gBundlesTraced.clear();
     }
 
     utrace_setFunctions(context, nullptr, nullptr, nullptr);