Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix potentially high CPU usage on Windows #4448

Merged
merged 11 commits into from
Mar 20, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#include <utility>
#include <vector>
/**
* @brief THe Cryptography class provides a set of basic cryptographic primatives required
* @brief The Cryptography class provides a set of basic cryptographic primatives required
* by the attestation samples.
*/

Expand Down
1 change: 1 addition & 0 deletions sdk/core/azure-core/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
### Bugs Fixed

- [[#4213]](https://github.com/Azure/azure-sdk-for-cpp/issues/4213) Fixed a bug where `Host` request header is not set for non-default port (80, 443).
- [[#4443]](https://github.com/Azure/azure-sdk-for-cpp/issues/4443) Fixed potentially high CPU usage on Windows.
antkmsft marked this conversation as resolved.
Show resolved Hide resolved

### Other Changes

Expand Down
16 changes: 16 additions & 0 deletions sdk/core/azure-core/inc/azure/core/internal/strings.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,22 @@ namespace Azure { namespace Core { namespace _internal {
return (c < 'A' || c > 'Z') ? c : c + ('a' - 'A');
}

static constexpr bool IsDigit(char c) noexcept { return c >= '0' && c <= '9'; }

static constexpr bool IsHexDigit(char c) noexcept
{
return IsDigit(c) || (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f');
}

static constexpr bool IsAlphaNumeric(char c) noexcept
{
return IsDigit(c) || (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
}

static constexpr bool IsSpace(char c) noexcept { return c == ' ' || (c >= '\t' && c <= '\r'); }
antkmsft marked this conversation as resolved.
Show resolved Hide resolved

static constexpr bool IsPrintable(char c) noexcept { return c >= ' ' && c <= '~'; }
antkmsft marked this conversation as resolved.
Show resolved Hide resolved

struct CaseInsensitiveComparator final
{
bool operator()(std::string const& lhs, std::string const& rhs) const
Expand Down
8 changes: 4 additions & 4 deletions sdk/core/azure-core/src/datetime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@
// SPDX-License-Identifier: MIT

#include "azure/core/datetime.hpp"
#include "azure/core/internal/strings.hpp"
#include "azure/core/platform.hpp"

#include <algorithm>
#include <ctime>
#include <iomanip>
#include <limits>
#include <locale>
#include <sstream>
#include <stdexcept>

Expand Down Expand Up @@ -320,7 +320,7 @@ T ParseNumber(
for (; i < MaxChars; ++i)
{
auto const ch = str[*cursor + i];
if (std::isdigit(ch, std::locale::classic()))
if (Core::_internal::StringExtensions::IsDigit(ch))
{
value = (value * 10) + (static_cast<decltype(value)>(static_cast<unsigned char>(ch)) - '0');
continue;
Expand Down Expand Up @@ -655,7 +655,7 @@ DateTime DateTime::Parse(std::string const& dateTime, DateFormat format)
if (charsRead == 7 && (DateTimeLength - cursor) > 0)
{
auto const ch = dateTime[cursor];
if (std::isdigit(ch, std::locale::classic()))
if (Core::_internal::StringExtensions::IsDigit(ch))
{
auto const num = static_cast<int>(static_cast<unsigned char>(ch) - '0');
if (num > 4)
Expand All @@ -677,7 +677,7 @@ DateTime DateTime::Parse(std::string const& dateTime, DateFormat format)

for (auto i = DateTimeLength - cursor; i > 0; --i)
{
if (std::isdigit(dateTime[cursor], std::locale::classic()))
if (Core::_internal::StringExtensions::IsDigit(dateTime[cursor]))
{
++minDateTimeLength;
++cursor;
Expand Down
4 changes: 2 additions & 2 deletions sdk/core/azure-core/src/http/curl/curl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "azure/core/http/policies/policy.hpp"
#include "azure/core/http/transport.hpp"
#include "azure/core/internal/diagnostics/log.hpp"
#include "azure/core/internal/strings.hpp"

// Private include
#include "curl_connection_pool_private.hpp"
Expand Down Expand Up @@ -70,7 +71,6 @@
#include <algorithm>
#include <chrono>
#include <iomanip>
#include <locale>
#include <sstream>
#include <string>
#include <thread>
Expand Down Expand Up @@ -1340,7 +1340,7 @@ void DumpCurlInfoToLog(std::string const& text, uint8_t* ptr, size_t size)
// Log the contents of the buffer as text, if it's printable, print the character, otherwise
// print '.'
auto const ch = static_cast<char>(ptr[i + c]);
if (std::isprint(ch, std::locale::classic()))
if (Azure::Core::_internal::StringExtensions::IsPrintable(ch))
{
ss << ch;
}
Expand Down
4 changes: 2 additions & 2 deletions sdk/core/azure-core/src/http/http.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@

#include "azure/core/http/http.hpp"
#include "azure/core/http/policies/policy.hpp"
#include "azure/core/internal/strings.hpp"
#include "azure/core/url.hpp"

#include <algorithm>
#include <locale>
#include <unordered_set>
#include <utility>

Expand All @@ -32,7 +32,7 @@ bool IsInvalidHeaderNameChar(char c)
static std::unordered_set<char> const HeaderNameExtraValidChars
= {' ', '!', '#', '$', '%', '&', '\'', '*', '+', '-', '.', '^', '_', '`', '|', '~'};

return !std::isalnum(c, std::locale::classic())
return !Azure::Core::_internal::StringExtensions::IsAlphaNumeric(c)
&& HeaderNameExtraValidChars.find(c) == HeaderNameExtraValidChars.end();
}
} // namespace
Expand Down
13 changes: 6 additions & 7 deletions sdk/core/azure-core/src/http/url.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

#include <algorithm>
#include <limits>
#include <locale>
#include <stdexcept>
#include <unordered_set>

Expand All @@ -22,7 +21,7 @@ Url::Url(std::string const& url)

if (schemePos != std::string::npos)
{
m_scheme = Azure::Core::_internal::StringExtensions::ToLower(url.substr(0, schemePos));
m_scheme = _internal::StringExtensions::ToLower(url.substr(0, schemePos));
urlIter += schemePos + SchemeEnd.length();
}
}
Expand All @@ -43,8 +42,8 @@ Url::Url(std::string const& url)
if (*urlIter == ':')
{
++urlIter;
auto const portIter = std::find_if_not(
urlIter, url.end(), [](auto c) { return std::isdigit(c, std::locale::classic()); });
auto const portIter
= std::find_if_not(urlIter, url.end(), _internal::StringExtensions::IsDigit);

auto const portNumber = std::stoi(std::string(urlIter, portIter));

Expand Down Expand Up @@ -105,8 +104,8 @@ std::string Url::Decode(std::string const& value)
{
case '%':
if ((valueSize - i) < 3 // need at least 3 characters: "%XY"
|| !std::isxdigit(value[i + 1], std::locale::classic())
|| !std::isxdigit(value[i + 2], std::locale::classic()))
|| !_internal::StringExtensions::IsHexDigit(value[i + 1])
|| !_internal::StringExtensions::IsHexDigit(value[i + 2]))
{
throw std::runtime_error("failed when decoding URL component");
}
Expand All @@ -133,7 +132,7 @@ bool ShouldEncode(char c)
{
static std::unordered_set<char> const ExtraNonEncodableChars = {'-', '.', '_', '~'};

return !std::isalnum(c, std::locale::classic())
return !_internal::StringExtensions::IsAlphaNumeric(c)
&& ExtraNonEncodableChars.find(c) == ExtraNonEncodableChars.end();
}
} // namespace
Expand Down
14 changes: 9 additions & 5 deletions sdk/core/azure-core/src/http/user_agent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@

#include "azure/core/context.hpp"
#include "azure/core/http/policies/policy.hpp"
#include "azure/core/internal/strings.hpp"
#include "azure/core/internal/tracing/service_tracing.hpp"
#include "azure/core/platform.hpp"
#include <locale>
#include <sstream>

#if defined(AZ_PLATFORM_WINDOWS)
Expand Down Expand Up @@ -133,10 +133,14 @@ std::string GetOSVersion()

std::string TrimString(std::string s)
{
auto const isNotSpace = [](char c) { return !std::isspace(c, std::locale::classic()); };

s.erase(s.begin(), std::find_if(s.begin(), s.end(), isNotSpace));
s.erase(std::find_if(s.rbegin(), s.rend(), isNotSpace).base(), s.end());
s.erase(
s.begin(),
std::find_if_not(s.begin(), s.end(), Azure::Core::_internal::StringExtensions::IsSpace));

s.erase(
std::find_if_not(s.rbegin(), s.rend(), Azure::Core::_internal::StringExtensions::IsSpace)
.base(),
s.end());

return s;
}
Expand Down
50 changes: 50 additions & 0 deletions sdk/core/azure-core/test/ut/string_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,56 @@ TEST(String, toUpperC)
}
}

TEST(String, isDigit)
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
{
using Azure::Core::_internal::StringExtensions;
for (unsigned i = 0; i <= 255; ++i)
{
auto const c = static_cast<char>(static_cast<unsigned char>(i));
EXPECT_EQ(StringExtensions::IsDigit(c), std::isdigit(c, std::locale::classic()));
}
}

TEST(String, isHexDigit)
{
using Azure::Core::_internal::StringExtensions;
for (unsigned i = 0; i <= 255; ++i)
{
auto const c = static_cast<char>(static_cast<unsigned char>(i));
EXPECT_EQ(StringExtensions::IsHexDigit(c), std::isxdigit(c, std::locale::classic()));
}
}

TEST(String, isAlphaNumeric)
{
using Azure::Core::_internal::StringExtensions;
for (unsigned i = 0; i <= 255; ++i)
{
auto const c = static_cast<char>(static_cast<unsigned char>(i));
EXPECT_EQ(StringExtensions::IsAlphaNumeric(c), std::isalnum(c, std::locale::classic()));
}
}

TEST(String, isSpace)
{
using Azure::Core::_internal::StringExtensions;
for (unsigned i = 0; i <= 255; ++i)
{
auto const c = static_cast<char>(static_cast<unsigned char>(i));
EXPECT_EQ(StringExtensions::IsSpace(c), std::isspace(c, std::locale::classic()));
}
}

TEST(String, isPrintable)
{
using Azure::Core::_internal::StringExtensions;
for (unsigned i = 0; i <= 255; ++i)
{
auto const c = static_cast<char>(static_cast<unsigned char>(i));
EXPECT_EQ(StringExtensions::IsPrintable(c), std::isprint(c, std::locale::classic()));
}
}

TEST(String, toLower)
{
using Azure::Core::_internal::StringExtensions;
Expand Down
2 changes: 2 additions & 0 deletions sdk/identity/azure-identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### Bugs Fixed

- [[#4443]](https://github.com/Azure/azure-sdk-for-cpp/issues/4443) Fixed potentially high CPU usage on Windows.

Comment on lines +11 to +12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

### Other Changes

- Improved diagnostics to utilize `Azure::Core::Credentials::TokenCredential::GetCredentialName()`.
Expand Down
5 changes: 3 additions & 2 deletions sdk/identity/azure-identity/src/azure_cli_credential.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@

#include <azure/core/internal/diagnostics/log.hpp>
#include <azure/core/internal/environment.hpp>
#include <azure/core/internal/strings.hpp>
#include <azure/core/internal/unique_handle.hpp>
#include <azure/core/platform.hpp>

#include <cstdio>
#include <locale>
#include <stdexcept>
#include <thread>
#include <type_traits>
Expand Down Expand Up @@ -40,6 +40,7 @@ using Azure::Identity::AzureCliCredential;
using Azure::DateTime;
using Azure::Core::Context;
using Azure::Core::_internal::Environment;
using Azure::Core::_internal::StringExtensions;
using Azure::Core::Credentials::AccessToken;
using Azure::Core::Credentials::AuthenticationException;
using Azure::Core::Credentials::TokenCredentialOptions;
Expand Down Expand Up @@ -71,7 +72,7 @@ void AzureCliCredential::ThrowIfNotSafeCmdLineInput(
break;

default:
if (!std::isalnum(c, std::locale::classic()))
if (!StringExtensions::IsAlphaNumeric(c))
{
throw AuthenticationException(
IdentityPrefix + GetCredentialName() + ": Unsafe command line input found in "
Expand Down