Fix potentially high CPU usage on Windows

[antkmsft]

This is a proposal to fix the underlying issue in #4443.
Instead of caching locale, I propose to have our own set of functions.
Closes #4443.

[LarryOsterman]

Now that I've had a few days to think about it, I don't really have a problem with it (especially given the test to verify that the current functions match the CRT behavior).

[RickWinter]

Is the perf better than a single cached reference to locale (which is immutable)?

[antkmsft]

@RickWinter, it appears to be 64x faster than the caching implementation:

Inline functions for the StringExtensions class:

static std::locale const& GetLocale()
{
    static auto const locale = std::locale::classic();
    return locale;
}

static bool IsDigit2(char c) noexcept { return std::isdigit(c, GetLocale()); }
static bool IsHexDigit2(char c) noexcept { return std::isxdigit(c, GetLocale()); }
static bool IsAlphaNumeric2(char c) noexcept { return std::isalnum(c, GetLocale()); }
static bool IsSpace2(char c) noexcept { return std::isspace(c, GetLocale()); }
static bool IsPrintable2(char c) noexcept { return std::isprint(c, GetLocale()); }

Test:

using Azure::Core::_internal::StringExtensions;
auto const iterations = 10000;

auto start = std::chrono::system_clock::now();
for (auto n = 0; n < iterations; ++n)
{
for (unsigned i = 0; i <= 255; ++i)
{
    auto const c = static_cast<char>(static_cast<unsigned char>(i));
    static_cast<void>(StringExtensions::IsDigit(c));
    static_cast<void>(StringExtensions::IsHexDigit(c));
    static_cast<void>(StringExtensions::IsAlphaNumeric(c));
    static_cast<void>(StringExtensions::IsSpace(c));
    static_cast<void>(StringExtensions::IsPrintable(c));
}
}

auto elapsed = std::chrono::system_clock::now() - start;
std::cout << "-------------\n\nHandwritten: "
        << static_cast<double>(
                std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count()) << "ms\n";

start = std::chrono::system_clock::now();
for (auto n = 0; n < iterations; ++n)
{
for (unsigned i = 0; i <= 255; ++i)
{
    auto const c = static_cast<char>(static_cast<unsigned char>(i));
    static_cast<void>(StringExtensions::IsDigit2(c));
    static_cast<void>(StringExtensions::IsHexDigit2(c));
    static_cast<void>(StringExtensions::IsAlphaNumeric2(c));
    static_cast<void>(StringExtensions::IsSpace2(c));
    static_cast<void>(StringExtensions::IsPrintable2(c));
}
}

elapsed = std::chrono::system_clock::now() - start;
std::cout << "\nStd-cached : "
        << static_cast<double>(
                std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count())
        << "ms\n\n-------------\n";

Results:

-------------

Handwritten: 34ms

Std-cached : 2181ms

-------------

[antkmsft]

And:

• 56x faster than std:: version that does no caching
• 6.8x faster than the std:: version that takes no locale (we wouldn't have used it because it is machine dependent, but good for comparison)

[ahsonkhan]

I don't think it is necessary to call this out in the identity changelog. The use-case here wouldn't impact the customer perf, since it was only happening in the error path of ThrowIfNotSafeCmdLineInput. That isn't a case where there would be a scalability issue causing high CPU usage.

[antkmsft]

I was thinking about that, but the change needs to be described. The customers want to know what they are getting when they are upgrading. Potentially, this still may lead to higher CPU usage, so I see no better description.

[antkmsft]

And it is not in the error path, it is a part of normal flow - when creating credential and during each call of GetToken().

[ahsonkhan]

I get that GetToken() might be transitively impacted (by implementation detail changes in core), but the only change that impacted identity, is the following which is in the error path:
ThrowIfNotSafeCmdLineInput