[GitHub] [trafficserver] duke8253 commented on a change in pull request #7501: JeMalloc performance improvments

[GitBox <gi...@apache.org>]

duke8253 commented on a change in pull request #7501:
URL: https://github.com/apache/trafficserver/pull/7501#discussion_r662507169



##########
File path: src/tscore/JeAllocator.cc
##########
@@ -52,44 +50,52 @@ JemallocNodumpAllocator::alloc(extent_hooks_t *extent, void *new_addr, size_t si
   return result;
 }
 
-#endif /* JEMALLOC_NODUMP_ALLOCATOR_SUPPORTED */
-
-bool
-JemallocNodumpAllocator::extend_and_setup_arena()
+unsigned int
+JemallocNodumpAllocator::extend_and_setup_arena(pthread_t thread_id)
 {
-#ifdef JEMALLOC_NODUMP_ALLOCATOR_SUPPORTED
-  size_t arena_index_len_ = sizeof(arena_index_);
-  if (auto ret = mallctl("arenas.create", &arena_index_, &arena_index_len_, nullptr, 0)) {
+  unsigned int arena_id;
+  size_t arena_id_len = sizeof(arena_id);
+  if (auto ret = mallctl("arenas.create", &arena_id, &arena_id_len, nullptr, 0)) {
     ink_abort("Unable to extend arena: %s", std::strerror(ret));
   }
-  flags_ = MALLOCX_ARENA(arena_index_) | MALLOCX_TCACHE_NONE;
+
+  int flags = MALLOCX_ARENA(arena_id) | MALLOCX_TCACHE_NONE;
 
   // Read the existing hooks
-  const auto key = "arena." + std::to_string(arena_index_) + ".extent_hooks";
+  const auto key = "arena." + std::to_string(arena_id) + ".extent_hooks";
   extent_hooks_t *hooks;
   size_t hooks_len = sizeof(hooks);
   if (auto ret = mallctl(key.c_str(), &hooks, &hooks_len, nullptr, 0)) {
     ink_abort("Unable to get the hooks: %s", std::strerror(ret));
   }
-  if (original_alloc_ == nullptr) {
-    original_alloc_ = hooks->alloc;
-  } else {
-    ink_release_assert(original_alloc_ == hooks->alloc);
-  }
 
-  // Set the custom hook
-  extent_hooks_             = *hooks;
-  extent_hooks_.alloc       = &JemallocNodumpAllocator::alloc;
-  extent_hooks_t *new_hooks = &extent_hooks_;
-  if (auto ret = mallctl(key.c_str(), nullptr, nullptr, &new_hooks, sizeof(new_hooks))) {
-    ink_abort("Unable to set the hooks: %s", std::strerror(ret));
+  {
+    std::unique_lock lock(je_mutex);
+    if (original_alloc == nullptr) {
+      original_alloc = hooks->alloc;
+    } else {
+      ink_release_assert(original_alloc == hooks->alloc);
+    }
+
+    // Set the custom hook
+    if (extent_hooks.alloc != &JemallocNodumpAllocator::alloc) {
+      extent_hooks       = *hooks;
+      extent_hooks.alloc = &JemallocNodumpAllocator::alloc;
+    }
+    extent_hooks_t *new_hooks = &extent_hooks;
+    if (auto ret = mallctl(key.c_str(), nullptr, nullptr, &new_hooks, sizeof(new_hooks))) {
+      ink_abort("Unable to set the hooks: %s", std::strerror(ret));
+    }
+
+    Debug("JeAllocator", "arena \"%ud\" created", arena_id);
+
+    arenas[thread_id]     = arena_id;

Review comment:
       I'm actually working on a version using `thread_local` as @bryancall suggested, https://github.com/duke8253/trafficserver/tree/master-jemalloc_local
   
   I'll merge this PR first since this is already a 2x improvement, then I'll create a new PR for the `thread_local` change.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org