[PATCH] oleaut32: Fix locale settings caching in VARIANT_GetLocalisedNumberChars().
Don't cache locale data coming from the registry because it could change at any time. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- Previously the settings of the first VarParseNumFromStr(LOCALE_USER_DEFAULT) call ended up being cached and used for all the remaining tests, thus causing many of them to fail. There's a comment that says the caching is because all these lookups are slow. And I guess the common case it likely to be the one that's no longer cached: i.e. getting the current user's preferences from the registry. But all in all that case does not seem that slow (roughly 1.1 us per call) so unless it's called millions of times it should be ok. Also checking the registry key's last modification time would not make it much faster (I expect at most x2). --- dlls/oleaut32/tests/vartest.c | 12 +++--------- dlls/oleaut32/variant.c | 16 +++++++++++----- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index bed4de469df..89859988f1f 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -2198,7 +2198,7 @@ static void test_VarParseNumFromStrMisc(void) /* But SMONDECIMALSEP has no default! */ SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_SMONDECIMALSEP, L""); hres = wconvert_str(L"3.9", ARRAY_SIZE(rgb), NUMPRS_DECIMAL|NUMPRS_CURRENCY|NUMPRS_USE_ALL, &np, rgb, LOCALE_USER_DEFAULT, 0); - todo_wine EXPECTFAIL; + EXPECTFAIL; SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_SDECIMAL, L"."); SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_SMONDECIMALSEP, L"."); @@ -2206,9 +2206,7 @@ static void test_VarParseNumFromStrMisc(void) SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_STHOUSAND, L" "); hres = wconvert_str(L"1 000", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS, &np, rgb, LOCALE_USER_DEFAULT, 0); - if (broken(1)) /* FIXME Reenable once Wine is less broken */ EXPECT(1,NUMPRS_THOUSANDS,NUMPRS_THOUSANDS,5,0,3); - todo_wine ok(np.dwOutFlags == NUMPRS_THOUSANDS, "Got dwOutFlags=%08x\n", np.dwOutFlags); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); @@ -2225,9 +2223,7 @@ static void test_VarParseNumFromStrMisc(void) SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_SMONTHOUSANDSEP, L" "); hres = wconvert_str(L"1|000", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS, &np, rgb, LOCALE_USER_DEFAULT, 0); - if (broken(1)) /* FIXME Reenable once Wine is less broken */ EXPECT(1,NUMPRS_THOUSANDS,NUMPRS_THOUSANDS,5,0,3); - todo_wine ok(np.dwOutFlags == NUMPRS_THOUSANDS, "Got dwOutFlags=%08x\n", np.dwOutFlags); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); @@ -2235,9 +2231,7 @@ static void test_VarParseNumFromStrMisc(void) EXPECTFAIL; hres = wconvert_str(L"1|000", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS|NUMPRS_CURRENCY, &np, rgb, LOCALE_USER_DEFAULT, 0); - if (broken(1)) /* FIXME Reenable once Wine is less broken */ EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY,NUMPRS_THOUSANDS,5,0,3); - todo_wine ok(np.dwOutFlags == NUMPRS_THOUSANDS, "Got dwOutFlags=%08x\n", np.dwOutFlags); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); @@ -2298,14 +2292,14 @@ static void test_VarParseNumFromStrMisc(void) SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_STHOUSAND, L"\xa0"); hres = wconvert_str(L"1 000", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL, &np, rgb, LOCALE_USER_DEFAULT, 0); - todo_wine EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL,NUMPRS_THOUSANDS,5,0,3); + EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL,NUMPRS_THOUSANDS,5,0,3); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); /* Regular thousands separators also have precedence over the currency ones */ hres = wconvert_str(L"1\xa0\x30\x30\x30", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL, &np, rgb, LOCALE_USER_DEFAULT, 0); - todo_wine EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL,NUMPRS_THOUSANDS,5,0,3); + EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL,NUMPRS_THOUSANDS,5,0,3); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c index e4993de75a5..15fc7f9042e 100644 --- a/dlls/oleaut32/variant.c +++ b/dlls/oleaut32/variant.c @@ -1529,8 +1529,11 @@ static void VARIANT_GetLocalisedNumberChars(VARIANT_NUMBER_CHARS *lpChars, LCID EnterCriticalSection(&cache_cs); /* Asking for default locale entries is very expensive: It is a registry - server call. So cache one locally, as Microsoft does it too */ - if(lcid == lastLcid && dwFlags == lastFlags) + * server call. So cache one locally, as Microsoft does it too. + * Note: Data in the registry could change at any time so only cache when + * LOCALE_NOUSEROVERRIDE is used. + */ + if (lctype && lcid == lastLcid && dwFlags == lastFlags) { memcpy(lpChars, &lastChars, sizeof(defaultChars)); LeaveCriticalSection(&cache_cs); @@ -1557,9 +1560,12 @@ static void VARIANT_GetLocalisedNumberChars(VARIANT_NUMBER_CHARS *lpChars, LCID TRACE("lcid 0x%x, cCurrencyLocal=%d,%d %s\n", lcid, lpChars->cCurrencyLocal, lpChars->cCurrencyLocal2, wine_dbgstr_w(buff)); - memcpy(&lastChars, lpChars, sizeof(defaultChars)); - lastLcid = lcid; - lastFlags = dwFlags; + if (lctype) + { + memcpy(&lastChars, lpChars, sizeof(defaultChars)); + lastLcid = lcid; + lastFlags = dwFlags; + } LeaveCriticalSection(&cache_cs); } -- 2.20.1
Somehow I did not receive the TestBot's email about the failures: https://testbot.winehq.org/JobDetails.pl?Key=94604#k402 But they are only preexisting typelib failures that are not impacted by this patch. https://test.winehq.org/data/patterns.html#oleaut32:typelib https://test.winehq.org/data/5dffe2263de41e76100ba0acd7c717267dc4c878/linux_... Btw, I looked into this typelib issue a bit but did not get anywhere. So if someone feels like tackling it... https://bugs.winehq.org//show_bug.cgi?id=51456 -- Francois Gouget <fgouget(a)codeweavers.com>
It turns out that the oleaut32 caching is not needed at all. I will send an updated patch. -- Francois Gouget <fgouget(a)codeweavers.com>
On Wed, 4 Aug 2021, Francois Gouget wrote:
It turns out that the oleaut32 caching is not needed at all. I will send an updated patch.
The attached patch can be used to collect more information about run times in each case: * If both the GetLocaleInfoW() and GetLocalisedNumberChars() caching are disabled (s/01/0/ in locale.c): GetLocalisedNumberChars(lcid=400,NOUSEROVERRIDE,cache=0) took 1422 ns GetLocalisedNumberChars(lcid=409,NOUSEROVERRIDE,cache=0) took 964 ns GetLocalisedNumberChars(lcid=40c,NOUSEROVERRIDE,cache=0) took 1012 ns GetLocalisedNumberChars(lcid=407,NOUSEROVERRIDE,cache=0) took 981 ns GetLocalisedNumberChars(lcid=400,0,cache=0) took 69000 ns GetLocalisedNumberChars(lcid=409,0,cache=0) took 1600 ns GetLocalisedNumberChars(lcid=40c,0,cache=0) took 68900 ns GetLocalisedNumberChars(lcid=407,0,cache=0) took 1600 ns Given that my locale is fr_FR.UTF-8, only the LOCALE_USER_DEFAULT (400) and LANG_FRENCH (40c) cases go through the registry. And that's where it's slow without caching: ~70000 ns per call. All the other cases just query the resources and are 40+ times faster. * If only the GetLocalisedNumberChars() caching is disabled: GetLocalisedNumberChars(lcid=400,NOUSEROVERRIDE,cache=0) took 1195 ns GetLocalisedNumberChars(lcid=409,NOUSEROVERRIDE,cache=0) took 820 ns GetLocalisedNumberChars(lcid=40c,NOUSEROVERRIDE,cache=0) took 809 ns GetLocalisedNumberChars(lcid=407,NOUSEROVERRIDE,cache=0) took 852 ns GetLocalisedNumberChars(lcid=400,0,cache=0) took 946 ns GetLocalisedNumberChars(lcid=409,0,cache=0) took 1156 ns GetLocalisedNumberChars(lcid=40c,0,cache=0) took 642 ns GetLocalisedNumberChars(lcid=407,0,cache=0) took 1157 ns Only the 400 and 40c cases go through the GetLocaleInfoW() registry cache. These are slightly faster than querying the resources but not by much (~650-950 ns instead of ~1150 ns). * And with GetLocalisedNumberChars() caching: GetLocalisedNumberChars(lcid=400,NOUSEROVERRIDE,cache=2) took 16 ns GetLocalisedNumberChars(lcid=409,NOUSEROVERRIDE,cache=2) took 16 ns GetLocalisedNumberChars(lcid=40c,NOUSEROVERRIDE,cache=2) took 32 ns GetLocalisedNumberChars(lcid=407,NOUSEROVERRIDE,cache=2) took 17 ns GetLocalisedNumberChars(lcid=400,0,cache=2) took 16 ns GetLocalisedNumberChars(lcid=409,0,cache=2) took 16 ns GetLocalisedNumberChars(lcid=40c,0,cache=2) took 16 ns GetLocalisedNumberChars(lcid=407,0,cache=2) took 16 ns GetLocalisedNumberChars()'s caching is the fastest by far (>70x) since it's a simple memcpy(). So with the patch submitted in this thread we keep the high performance when querying anything but the registry locale settings. But as mentioned before it's not clear how often that would be used. Maybe it can benefit some applications that parse a lot of data that's not in the user's locale (maybe because it comes from some database or spreadsheet). But if we consider GetLocaleInfoW()'s performance good enough then the GetLocalisedNumberChars() caching can be removed entirely. -- Francois Gouget <fgouget(a)codeweavers.com>
GetLocaleInfoW() already caches the registry information and is fast enough. This also avoids using out-of-date information after a SetLocaleInfoW() call. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- GetLocaleInfoW() already caches the registry data which provides a 40x speedup over the no-cache case: https://www.winehq.org/pipermail/wine-devel/2021-August/192041.html If that performance is considered sufficient this alternative to the above patch can be used. Its main advantage is that it simplifies the code by totally removing caching from VARIANT_GetLocalisedNumberChars(). --- dlls/oleaut32/tests/vartest.c | 12 +++--------- dlls/oleaut32/variant.c | 28 ---------------------------- 2 files changed, 3 insertions(+), 37 deletions(-) diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index bed4de469df..89859988f1f 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -2198,7 +2198,7 @@ static void test_VarParseNumFromStrMisc(void) /* But SMONDECIMALSEP has no default! */ SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_SMONDECIMALSEP, L""); hres = wconvert_str(L"3.9", ARRAY_SIZE(rgb), NUMPRS_DECIMAL|NUMPRS_CURRENCY|NUMPRS_USE_ALL, &np, rgb, LOCALE_USER_DEFAULT, 0); - todo_wine EXPECTFAIL; + EXPECTFAIL; SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_SDECIMAL, L"."); SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_SMONDECIMALSEP, L"."); @@ -2206,9 +2206,7 @@ static void test_VarParseNumFromStrMisc(void) SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_STHOUSAND, L" "); hres = wconvert_str(L"1 000", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS, &np, rgb, LOCALE_USER_DEFAULT, 0); - if (broken(1)) /* FIXME Reenable once Wine is less broken */ EXPECT(1,NUMPRS_THOUSANDS,NUMPRS_THOUSANDS,5,0,3); - todo_wine ok(np.dwOutFlags == NUMPRS_THOUSANDS, "Got dwOutFlags=%08x\n", np.dwOutFlags); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); @@ -2225,9 +2223,7 @@ static void test_VarParseNumFromStrMisc(void) SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_SMONTHOUSANDSEP, L" "); hres = wconvert_str(L"1|000", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS, &np, rgb, LOCALE_USER_DEFAULT, 0); - if (broken(1)) /* FIXME Reenable once Wine is less broken */ EXPECT(1,NUMPRS_THOUSANDS,NUMPRS_THOUSANDS,5,0,3); - todo_wine ok(np.dwOutFlags == NUMPRS_THOUSANDS, "Got dwOutFlags=%08x\n", np.dwOutFlags); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); @@ -2235,9 +2231,7 @@ static void test_VarParseNumFromStrMisc(void) EXPECTFAIL; hres = wconvert_str(L"1|000", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS|NUMPRS_CURRENCY, &np, rgb, LOCALE_USER_DEFAULT, 0); - if (broken(1)) /* FIXME Reenable once Wine is less broken */ EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY,NUMPRS_THOUSANDS,5,0,3); - todo_wine ok(np.dwOutFlags == NUMPRS_THOUSANDS, "Got dwOutFlags=%08x\n", np.dwOutFlags); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); @@ -2298,14 +2292,14 @@ static void test_VarParseNumFromStrMisc(void) SetLocaleInfoW(LOCALE_USER_DEFAULT, LOCALE_STHOUSAND, L"\xa0"); hres = wconvert_str(L"1 000", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL, &np, rgb, LOCALE_USER_DEFAULT, 0); - todo_wine EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL,NUMPRS_THOUSANDS,5,0,3); + EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL,NUMPRS_THOUSANDS,5,0,3); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); /* Regular thousands separators also have precedence over the currency ones */ hres = wconvert_str(L"1\xa0\x30\x30\x30", ARRAY_SIZE(rgb), NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL, &np, rgb, LOCALE_USER_DEFAULT, 0); - todo_wine EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL,NUMPRS_THOUSANDS,5,0,3); + EXPECT(1,NUMPRS_THOUSANDS|NUMPRS_CURRENCY|NUMPRS_USE_ALL,NUMPRS_THOUSANDS,5,0,3); EXPECTRGB(0,1); /* Don't test extra digits, see "1,000" test */ EXPECTRGB(4,FAILDIG); diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c index e4993de75a5..14ba63b9b2e 100644 --- a/dlls/oleaut32/variant.c +++ b/dlls/oleaut32/variant.c @@ -42,14 +42,6 @@ WINE_DEFAULT_DEBUG_CHANNEL(variant); -static CRITICAL_SECTION cache_cs; -static CRITICAL_SECTION_DEBUG critsect_debug = -{ - 0, 0, &cache_cs, - { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList }, - 0, 0, { (DWORD_PTR)(__FILE__ ": cache_cs") } -}; -static CRITICAL_SECTION cache_cs = { &critsect_debug, -1, 0, 0, 0, 0 }; /* Convert a variant from one type to another */ static inline HRESULT VARIANT_Coerce(VARIANTARG* pd, LCID lcid, USHORT wFlags, @@ -1519,24 +1511,9 @@ HRESULT WINAPI VarUdateFromDate(DATE dateIn, ULONG dwFlags, UDATE *lpUdate) static void VARIANT_GetLocalisedNumberChars(VARIANT_NUMBER_CHARS *lpChars, LCID lcid, DWORD dwFlags) { static const VARIANT_NUMBER_CHARS defaultChars = { '-','+','.',',','$',0,'.',',' }; - static VARIANT_NUMBER_CHARS lastChars; - static LCID lastLcid = -1; - static DWORD lastFlags = 0; LCTYPE lctype = dwFlags & LOCALE_NOUSEROVERRIDE; WCHAR buff[4]; - /* To make caching thread-safe, a critical section is needed */ - EnterCriticalSection(&cache_cs); - - /* Asking for default locale entries is very expensive: It is a registry - server call. So cache one locally, as Microsoft does it too */ - if(lcid == lastLcid && dwFlags == lastFlags) - { - memcpy(lpChars, &lastChars, sizeof(defaultChars)); - LeaveCriticalSection(&cache_cs); - return; - } - memcpy(lpChars, &defaultChars, sizeof(defaultChars)); GET_NUMBER_TEXT(LOCALE_SNEGATIVESIGN, cNegativeSymbol); GET_NUMBER_TEXT(LOCALE_SPOSITIVESIGN, cPositiveSymbol); @@ -1556,11 +1533,6 @@ static void VARIANT_GetLocalisedNumberChars(VARIANT_NUMBER_CHARS *lpChars, LCID } TRACE("lcid 0x%x, cCurrencyLocal=%d,%d %s\n", lcid, lpChars->cCurrencyLocal, lpChars->cCurrencyLocal2, wine_dbgstr_w(buff)); - - memcpy(&lastChars, lpChars, sizeof(defaultChars)); - lastLcid = lcid; - lastFlags = dwFlags; - LeaveCriticalSection(&cache_cs); } /* Number Parsing States */ -- 2.20.1
participants (1)
-
Francois Gouget