From 77ca77484075a5e2816ddbe058e1498578aa3371 Mon Sep 17 00:00:00 2001 From: Shane Carr Date: Tue, 18 Jun 2019 13:33:49 -0700 Subject: [PATCH] ICU-20593 Various updates to resource tracing. --- docs/userguide/icu_data/tracing.md | 40 ++++++++++++++---------- icu4c/source/common/restrace.cpp | 33 +++++++++++++++----- icu4c/source/common/restrace.h | 19 ++++++++++-- icu4c/source/common/unicode/utrace.h | 38 ++++++++++++++++++----- icu4c/source/common/uresbund.cpp | 4 +++ icu4c/source/common/utrace.cpp | 11 ++++--- icu4c/source/test/intltest/restsnew.cpp | 41 ++++++++++++------------- 7 files changed, 128 insertions(+), 58 deletions(-) diff --git a/docs/userguide/icu_data/tracing.md b/docs/userguide/icu_data/tracing.md index adc57a9f776..3a300e25385 100644 --- 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 index 2ab82415f4b..5c6498850e2 100644 --- 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 index a8af9190cd5..ef29eaed578 100644 --- a/icu4c/source/common/restrace.h +++ b/icu4c/source/common/restrace.h @@ -66,6 +66,17 @@ public: ~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; @@ -73,9 +84,9 @@ private: 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 @@ public: 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 index 412e11ad2bc..1dfbed49dea 100644 --- 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 index b8688aac24a..124ed5b966d 100644 --- 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 index eced03b8bac..c9815465947 100644 --- 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 index 27b5957a56f..6f86709af88 100644 --- a/icu4c/source/test/intltest/restsnew.cpp +++ b/icu4c/source/test/intltest/restsnew.cpp @@ -1356,7 +1356,7 @@ void NewResourceBundleTest::TestFilter() { static std::vector gResourcePathsTraced; static std::vector gDataFilesTraced; -static std::vector gResFilesTraced; +static std::vector 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 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);