[kudu] branch master updated: [security] avoid allocating string in SimpleAcl::UserAllowed()

[al...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git


The following commit(s) were added to refs/heads/master by this push:
     new 43905d8  [security] avoid allocating string in SimpleAcl::UserAllowed()
43905d8 is described below

commit 43905d86bdbd2dd4a4bf9d0cea52453a3a7d9d31
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Mon Feb 28 16:47:36 2022 -0800

    [security] avoid allocating string in SimpleAcl::UserAllowed()
    
    While looking at diagnostic logs collected from a busy Kudu cluster,
    I noticed traces like the following:
    
      tids=[1902541,1902525,1902599]
          0x7f0f825725d0 <unknown>
                0xa986dd tcmalloc::internal::SpinLockDelay()
                0xa985ef SpinLock::SlowLock()
                0xa8e198 tcmalloc::CentralFreeList::InsertRange()
                0xa977a1 tcmalloc::ThreadCache::ReleaseToCentralCache()
                0xa97a90 tcmalloc::ThreadCache::Scavenge()
               0x217ca04 kudu::security::SimpleAcl::UserAllowed()
                0xaace27 kudu::server::ServerBase::Authorize()
               0x215191c kudu::rpc::GeneratedServiceIf::Handle()
               0x2152619 kudu::rpc::ServicePool::RunThread()
               0x22d570f kudu::Thread::SuperviseThread()
    
    The issue is a high contention in the libtcmalloc, but there is one more
    point here: Scavenge() in tcmalloc's ThreadCache is called from the
    inlined method ThreadCache::Deallocate(), so an instance of std::string
    is allocated every time when ContainsKey(users_, "*") is called.
    
    SimpleAcl::UserAllowed() is in the hot path since it's invoked upon
    processing an RPC to any of existing Kudu services. This patch
    introduces a static SimpleAcl::kWildcardAll member to use for the
    lookup in SimpleAcl::UserAllowed() and in other places.
    
    Change-Id: Ic9ed6140cbcf0487ec76b9f769cdf24f9de7b52c
    Reviewed-on: http://gerrit.cloudera.org:8080/18283
    Tested-by: Kudu Jenkins
    Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
 src/kudu/security/simple_acl.cc | 12 ++++++++----
 src/kudu/security/simple_acl.h  |  2 ++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/kudu/security/simple_acl.cc b/src/kudu/security/simple_acl.cc
index 9157bff..2cc1385 100644
--- a/src/kudu/security/simple_acl.cc
+++ b/src/kudu/security/simple_acl.cc
@@ -34,6 +34,8 @@ using std::vector;
 namespace kudu {
 namespace security {
 
+const std::string SimpleAcl::kWildcardAny = "*";
+
 SimpleAcl::SimpleAcl() {
 }
 
@@ -44,10 +46,12 @@ Status SimpleAcl::ParseFlag(const string& flag) {
   vector<StringPiece> fields = strings::Split(flag, ",", strings::SkipWhitespace());
   set<string> users;
   for (const auto& field : fields) {
-    if (field.empty()) continue;
+    if (field.empty()) {
+      continue;
+    }
     // if any field is a wildcard, no need to include the rest.
-    if (flag == "*") {
-      Reset({"*"});
+    if (flag == kWildcardAny) {
+      Reset({kWildcardAny});
       return Status::OK();
     }
 
@@ -81,7 +85,7 @@ void SimpleAcl::Reset(set<string> users) {
 }
 
 bool SimpleAcl::UserAllowed(const string& username) const {
-  return ContainsKey(users_, "*") || ContainsKey(users_, username);
+  return ContainsKey(users_, kWildcardAny) || ContainsKey(users_, username);
 }
 
 } // namespace security
diff --git a/src/kudu/security/simple_acl.h b/src/kudu/security/simple_acl.h
index 9216a1a..502bcd3 100644
--- a/src/kudu/security/simple_acl.h
+++ b/src/kudu/security/simple_acl.h
@@ -50,6 +50,8 @@ class SimpleAcl {
   void Reset(std::set<std::string> users);
 
  private:
+  static const std::string kWildcardAny;
+
   // The set of users, or a set with the single value '*' for the wildcard.
   std::set<std::string> users_;
 };