Page MenuHomePhabricator

Speed up certain locale functions on Windows
ClosedPublic

Authored by thomasanderson on Mar 18 2019, 5:37 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

thomasanderson created this revision.Mar 18 2019, 5:37 PM
thomasanderson edited the summary of this revision. (Show Details)Mar 18 2019, 5:43 PM
smeenai added inline comments.
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 marked an inline comment as done.Mar 19 2019, 1:04 PM
thomasanderson added inline comments.
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 accepted this revision.Mar 19 2019, 1:11 PM

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.

This revision is now accepted and ready to land.Mar 19 2019, 1:11 PM

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 marked 2 inline comments as done.
thomasanderson edited the summary of this revision. (Show Details)

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

Done.

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!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2019, 1:31 PM

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

Why did you commit it?

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.

@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.

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

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.

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.)

This LGTM. Thanks for the contribution.