Speed up certain locale functions on Windows

[thomasanderson]

Summary

The issue is that __libcpp_locale_guard makes some slow calls to setlocale().
This change avoids using __libcpp_locale_guard in snprintf_l().

Fixes https://bugs.llvm.org/show_bug.cgi?id=41131

[smeenai]

include/__locale

67 ↗

(On Diff #191218)

I'm not really familiar with locales, but if I'm reading MSDN correctly, wouldn't you need a call with a NULL locale argument to get the value of all categories? I'm not sure how you'd then use that to restore the categories though.

Also, "Global or thread local storage is used for the string returned by setlocale. Later calls to setlocale overwrite the string, which invalidates string pointers returned by earlier calls." Shouldn't we be copying the return value instead of just storing a pointer? (This is already potentially a problem with the existing code, unless I'm misunderstanding things.)

[thomasanderson]

include/__locale

67 ↗

(On Diff #191218)

Yeah, you're right. How was this ever working? haha

I've reverted the __locale changes. I'll try to write a fix in a separate CL. For now, the change in locale_win32.cpp is all that's needed to fix Chromium's perf issues after switching to libc++ on Windows.

[smeenai]

LGTM, though you'll wanna update the commit message.

src/support/win32/locale_win32.cpp

96 ↗

(On Diff #191376)

Any reason for preferring this over _vsnprintf_l? I think they'll end up being basically equivalent here, except _vsnprintf_l is more in line with the else branch.

[smeenai]

Unrelated, but out of curiosity: I believe libc++ is set up to require building against the Windows 10 SDK and the Universal CRT right now. For older Windows versions, does Chrome require the Universal CRT to be installed?

[thomasanderson]

In D59525#1435377, @smeenai wrote:

LGTM, though you'll wanna update the commit message.

Done.

In D59525#1435389, @smeenai wrote:

Unrelated, but out of curiosity: I believe libc++ is set up to require building against the Windows 10 SDK and the Universal CRT right now. For older Windows versions, does Chrome require the Universal CRT to be installed?

No, I believe we statically link the CRT

src/support/win32/locale_win32.cpp

96 ↗

(On Diff #191376)

I just found out about _vsprintf_s_l first. Changed!

[mclow.lists]

Thomas - reviews for libc++ must be approved by either @EricWF, @ldionne or myself. None of them approved this.

Why did you commit it?

[thomasanderson]

In D59525#1435442, @mclow.lists wrote:

Thomas - reviews for libc++ must be approved by either @EricWF, @ldionne or myself. None of them approved this.

Why did you commit it?

The CL said "approved, ready to land". Clearly I'm not very good at this. Please go ahead and revert and/or revoke my committer permissions. Whatever you gotta do.

[smeenai]

@mclow.lists – this is a small localized change specific to libc++'s Windows support. The only libc++ specific thing here is the use of the _LIBCPP_MSVCRT conditionals, but they're consistent with the rest of this file (and with the general intent to support non-msvcrt runtimes on Windows). I've reviewed and contributed a bunch of libc++ Windows support, so I considered myself to be a qualified reviewer for this change, although for this particular change I think anyone sufficiently familiar with msvcrt would be able to review it.

I'm happy to not accept diffs in libc++ directly anymore and instead comment something like "LGTM, but wait for one of @mclow.lists, @EricWF, or @ldionne to review as well", but that seems like a pretty heavy handed policy for changes like this. I'm also curious if you have any specific issues with this diff, or if it's just a general principle.

[smeenai]

In any case, I apologize for the confusion, @thomasanderson – this isn't on you :)

[mclow.lists]

In D59525#1435566, @smeenai wrote:

I'm happy to not accept diffs in libc++ directly anymore and instead comment something like "LGTM, but wait for one of @mclow.lists, @EricWF, or @ldionne to review as well", but that seems like a pretty heavy handed policy for changes like this. I'm also curious if you have any specific issues with this diff, or if it's just a general principle.

It's definitely a general principle, not a comment on this diff.

[thakis]

In D59525#1435442, @mclow.lists wrote:

Thomas - reviews for libc++ must be approved by either @EricWF, @ldionne or myself. None of them approved this.

Why did you commit it?

https://libcxx.llvm.org has a section saying "Get it and get involved! First please review our Developer's Policy." that links to https://llvm.org/docs/DeveloperPolicy.html That page has a section https://llvm.org/docs/DeveloperPolicy.html#code-reviews that says "Code can be reviewed either before it is committed or after. We expect major changes to be reviewed before being committed, but smaller changes (or changes where the developer owns the component) can be reviewed after commit.". If libcxx's review policy is different from what it's documented as being, consider updating the documentation.

(The next section, https://llvm.org/docs/DeveloperPolicy.html#code-owners , talks about codifying the code owners policy a bit; libcxx should probably have a CODE_OWNERS.txt file like other llvm projects.)

[EricWF]

This LGTM. Thanks for the contribution.

libcxx/trunk/src/support/win32/locale_win32.cpp

 int wctob_l( wint_t c, locale_t loc )
 {
     __libcpp_locale_guard __current(loc);
     return wctob( c );
 }
 
 int snprintf_l(char *ret, size_t n, locale_t loc, const char *format, ...)
 {
+#if !defined(_LIBCPP_MSVCRT)
     __libcpp_locale_guard __current(loc);
+#endif
     va_list ap;
     va_start( ap, format );
+#if defined(_LIBCPP_MSVCRT)
+    int result = _vsnprintf_l( ret, n, format, loc, ap );
+#else
     int result = vsnprintf( ret, n, format, ap );
+#endif
     va_end(ap);
     return result;
 }
 
 int asprintf_l( char **ret, locale_t loc, const char *format, ... )
 {
     va_list ap;
     va_start( ap, format );