Not done but thought this looked like an interesting one to send comments in on.....

This looks like a useful change. Please let me know if you have any questions. Thanks!

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @arjunroy and @vjpai)

src/core/lib/gpr/arena.h, line 54 at r2 (raw file):

// may be useful for debugging purposes.
//#define SIMPLE_ARENA_FOR_DEBUGGING
#ifdef SIMPLE_ARENA_FOR_DEBUGGING

I would ideally prefer to find a way to keep this macro in the .cc file. It's not clear to me why it needs to be moved here; the struct definition can be in the .h file regardless of whether or not this macro is defined, because the API should be the same in both cases.

If this does stay here, it needs a GPR_ prefix, to avoid polluting the global C preprocessor macro namespace.

Another alternative might be to just remove the simple arena implementation. I added it a long time ago when I was trying to debug something, and I haven't used it since. And I'm not sure if anyone else has ever found it useful for anything, although you can ask around to see if I'm wrong about that.

src/core/lib/gpr/arena.h, line 60 at r2 (raw file):

#else  // SIMPLE_ARENA_FOR_DEBUGGING

struct gpr_arena {

If we're converting this to C++, let's go ahead and make this a class. We should also probably move this code from gpr to gprpp.

src/core/lib/gpr/arena.h, line 61 at r2 (raw file):

struct gpr_arena {
  struct Zone {

This should be private within the arena class.

src/core/lib/gpr/arena.h, line 65 at r2 (raw file):

  };

  gpr_arena(size_t initial_size) : initial_zone_size(initial_size) {}

Single-argument ctors must be declared explicit, as per the style guide.

src/core/lib/gpr/arena.h, line 75 at r2 (raw file):

    }
  }
  void* Alloc(size_t size) {

If we're exposing methods like this, then let's change callers to use this API directly instead of maintaining the existing C function API.

src/core/lib/gpr/arena.h, line 95 at r2 (raw file):

  // Keep track of the total used size. We use this in our call sizing
  // hysteresis.
  gpr_atm total_used = 0;

Could change this to use grpc_core::Atomic<>.

src/core/lib/gpr/arena.h, line 95 at r2 (raw file):

  // Keep track of the total used size. We use this in our call sizing
  // hysteresis.
  gpr_atm total_used = 0;

Class data members should have a _ suffix.

src/core/lib/gpr/arena.cc, line 155 at r2 (raw file):

      GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(Zone)) + size)) Zone();
  {
    while (arena_growth_spinlock.test_and_set(std::memory_order_acquire)) {

Doesn't this spinlock perform worse than a mutex when there's contention? Is the assumption here that contention should be rare, so this is a worthwhile trade-off for the memory savings?

Reviewable status: 0 of 2 files reviewed, 11 unresolved discussions (waiting on @markdroth and @vjpai)

src/core/lib/gpr/arena.h, line 60 at r1 (raw file):

Done.

src/core/lib/gpr/arena.h, line 54 at r2 (raw file):

The main reason it's here now is that it is changing whether the alloc() method is inlined here (normally) or defined in the arena.cc file. We want it to be inlined in the header file, but there are fundamentally two implementations of the same method, and hence the ifdef is here.

I don't see how we can remove the macro from the header and still have the method be defined.

At this point I think the arena is mature enough that we can just remove the simple implementation and the macro.

src/core/lib/gpr/arena.h, line 60 at r2 (raw file):

I converted it to a class; but for this patch I'll leave it in gpr and set a TODO to move it to gprpp.

src/core/lib/gpr/arena.h, line 61 at r2 (raw file):

Done.

src/core/lib/gpr/arena.h, line 65 at r2 (raw file):

Done.

src/core/lib/gpr/arena.h, line 75 at r2 (raw file):

Done.

src/core/lib/gpr/arena.h, line 95 at r2 (raw file):

Done.

src/core/lib/gpr/arena.h, line 95 at r2 (raw file):

Done.

src/core/lib/gpr/arena.cc, line 155 at r2 (raw file):

In case there's contention, I think it's still an overall win to use a spinlock here since all we're doing is setting two words (one of which is already in cache).

In the case of the mutex, assuming pthread_mutex_t is underneath the specific realization of grpc, we'll have the added overhead of the call/ret/register saving/etc. and the thread may sleep, leading to a particularly long wait compared to the time it takes a thread holding the spinlock to run the pointer swap.

This is definitely moving in the right direction!

Please let me know if you have any questions.

Reviewed 8 of 11 files at r3, 7 of 7 files at r4.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, and @vjpai)

src/core/ext/filters/client_channel/health/health_check_client.cc, line 284 at r3 (raw file):

      health_check_client_(std::move(health_check_client)),
      pollent_(grpc_polling_entity_create_from_pollset_set(interested_parties)),
      arena_(grpc_core::Arena::Create(health_check_client_

No need for the grpc_core:: prefix here; we're already in that namespace.

src/core/lib/gpr/arena.h, line 60 at r2 (raw file):

Any reason not to move it as part of this PR? Doesn't seem hard to do.

src/core/lib/gpr/arena.h, line 46 at r3 (raw file):

class Arena {
 public:
  static Arena* Create(size_t initial_size);

It seems a little cumbersome to have a factory method that returns a bare pointer (not a smart pointer) and a Destroy() method that is different from the dtor. This means that ownership of the arena needs to be handled manually. I'd prefer a mechanism whereby the compiler can enforce this for us.

Ideally, I'd like us to just directly expose the ctor and dtor, and let the arena be a direct member of whatever struct contains it. Note that this would require the Arena object itself to be allocated separately from the initial zone. Is that likely to have negative impact on the cache behavior by having them be in non-contiguous memory locations?

If that won't work, my next preference would be to expose the dtor and have the factory method return a UniquePtr<>, so that the arena will get cleaned up automatically. The problem with this is that we're using gpr_malloc_aligned() to create the arena, which means the UniquePtr<> needs to know to use gpr_free_aligned() to free it. I'm not sure if there's a reasonable way to do that.

If neither of those approaches can be done easily, then I can live with leaving this as-is. But if there is a way to clean this up, that would be great.

src/core/lib/gpr/arena.h, line 47 at r3 (raw file):

 public:
  static Arena* Create(size_t initial_size);
  size_t Destroy();

Note that if we do wind up taking one of the approaches above to directly expose the dtor, then we don't need this method. Instead, we can expose a method to get the total amount allocated by the arena (like the Used() method you had in the previous iteration) and require callers to call that method before destroying the arena.

src/core/lib/gpr/arena.h, line 49 at r3 (raw file):

  size_t Destroy();

  void* Alloc(size_t size) {

One of the most common patterns for calling this is:

MyType* t = static_cast<MyType*>(arena->Alloc(sizeof(MyType));
new (t) MyType(args...);

It would make the API easier to use if we provided a templated version of this method that makes this easier for callers:

template <typename T, typename... Args>
T* Alloc(Args&&... args) {
  T* t = static_cast<T*>(Alloc(sizeof(T)));
  new (t) T(std::forward<Args>(args)...);
  return t;
}

That way, callers can just do this:

MyType* p = arena->Alloc<MyType>(args...);

This might also allow us to address the TODO on line 51 of the .cc file about being smarter about the alignment requirements for each allocation. What do you think?

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

}  // namespace grpc_core

typedef class grpc_core::Arena gpr_arena;

Let's remove this C-style API (both the typedef and the functions below that use it). We should convert all callers to use the C++ API directly.

src/core/lib/gpr/arena.cc, line 150 at r1 (raw file):

Still need to correct this spelling error (although it's a pre-existing problem).

src/core/lib/gpr/arena.cc, line 57 at r3 (raw file):

// would allow us to use the alignment actually needed by the caller.
Arena* Arena::Create(size_t initial_size) {
  static constexpr size_t base_size =

This same line exists in the Alloc() method in the .h file. Maybe make this a static class member, so it only needs to be defined once?

src/core/lib/gpr/arena.cc, line 61 at r3 (raw file):

  initial_size = GPR_ROUND_UP_TO_ALIGNMENT_SIZE(initial_size);
  size_t alloc_size =  base_size + initial_size;
  return new (gpr_arena_malloc<GPR_CACHELINE_SIZE>(alloc_size))

If we're rounding up all allocations to the cache line size but using GPR_ROUND_UP_TO_ALIGNMENT_SIZE() for all of the padding in this code, is that going to cause problems?

We've had a lot of subtle bugs caused by alignment issues in the past, so I want to be very sure we're not going to break anything here.

src/core/lib/security/context/security_context.h, line 32 at r4 (raw file):

namespace grpc_core {
class Arena;

Please prefer including the necessary header file over adding a forward declaration.

Haven't had a chance to address all the comments yet, but I do have one specific comment I want to address now re: breaking the structure up (which I do not think we should do). See inline.

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, @markdroth, and @vjpai)

src/core/ext/filters/client_channel/health/health_check_client.cc, line 284 at r3 (raw file):

Done.

src/core/lib/gpr/arena.h, line 46 at r3 (raw file):

I do not support allocating the arena differently from the initial zone.

In the common case of allocating in the initial zone, we'd have an extra mov instruction (and read of something in memory) to get at the pointer we want. https://godbolt.org/z/shTAbf is a minimalized example (get1() has the extra read compared to what we're doing now with get2()).

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

The open source users have all been converted. I wanted to leave these methods in for transitioning any non-open users.

src/core/lib/gpr/arena.cc, line 36 at r1 (raw file):

Done.

src/core/lib/gpr/arena.cc, line 150 at r1 (raw file):

Done.

src/core/lib/gpr/arena.cc, line 57 at r3 (raw file):

Unable - if it's a static constexpr, in the class definition, it will complain because I'm using sizeof and sizeof isn't known till the class definition is ended (ie. "};"). But because it's constexpr, I can't declare it in the class and define it outside, because it wants an initializer then and there.

src/core/lib/gpr/arena.cc, line 61 at r3 (raw file):

I do not think this will cause a problem if GPR_CACHELINE_SIZE is larger than GPR_MAX_ALIGNMENT.
I suppose we could either do a static_assert that this is true, or leverage a constexpr that checks at compile time and picks the larger of the two.

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, @markdroth, and @vjpai)

src/core/lib/gpr/arena.h, line 49 at r3 (raw file):

+1 I would suggest having two variants of C-like and C++-like allocation:void* Alloc(size) and template <T, ...> T* New(...)

src/core/lib/gpr/arena.cc, line 57 at r3 (raw file):

you can potentially make that a constexpr method: https://godbolt.org/z/5uP5ES
but we need to make sure all supported compilers support that.

This looks really good. Thank you Arjun!

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, @markdroth, and @vjpai)

Updated diff with changes for comments from Mark and Soheil.

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @apolcyn, @AspirinSJL, @markdroth, and @vjpai)

src/core/lib/gpr/arena.h, line 60 at r2 (raw file):

Done.

src/core/lib/gpr/arena.h, line 46 at r3 (raw file):

Also, the main user for the arena is grpc_call. The arena is, however, not contained by grpc_call; rather it contains the grpc_call. In Soviet Russia, etc, etc...

Anyways, while UniquePtr<> is workable, per discussion with Mark we've decided to hold off for now.

src/core/lib/gpr/arena.h, line 47 at r3 (raw file):

Per discussion with Mark, holding off for now.

src/core/lib/gpr/arena.h, line 49 at r3 (raw file):

Done, and converted the callers that already use placement new in conjunction with gpr_arena_alloc of exactly the sizeof() the struct in question.

Did not convert:

• instances not using placement new
• instances with additional buffer after (eg. allocating grpc_call)
• instances where what we want is an array, not a single object.

src/core/lib/gpr/arena.cc, line 57 at r3 (raw file):

Maybe, but I feel that's not really cleaning things up too much so I'm in favour of just leaving it be for now.

src/core/lib/gpr/arena.cc, line 61 at r3 (raw file):

Done; added a constexpr alignment that chooses GPR_CACHELINE_SIZE iff. GPR_CACHELINE_SIZE > GPR_MAX_ALIGNMENT && GPR_CACHELINE_SIZE % GPR_MAX_ALIGNMENT == 0. So we'll always have GPR_MAX_ALIGNMENT, and will also have GPR_CACHELINE_SIZE alignment on reasonable architectures.

src/core/lib/security/context/security_context.h, line 32 at r4 (raw file):

Done.

This looks great! Just a couple of minor comments remaining.

Reviewed 23 of 23 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, @soheilhy, and @vjpai)

src/core/lib/gpr/arena.h, line 49 at r3 (raw file):

This looks great! Can we also make use of this new API to be smarter about alignment, or is that not straightforward?

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

I don't think you've actually converted all of the references to these types in OSS. I noticed at least one instance of gpr_arena in subchannel.h. And as long as these definitions are here, the build won't fail if anything else is referencing them, so it's hard to be sure.

In any case, these are internal APIs, not public ones. There should not be any external callers, so it should be safe to get rid of them.

There may be some internal code that we need to modify once this gets imported, but you can use codesearch to check. If there is any such code, we can import this as a cherry-pick that updates the internal callers at the same time.

Reviewable status: 24 of 32 files reviewed, 5 unresolved discussions (waiting on @apolcyn, @AspirinSJL, @markdroth, and @vjpai)

src/core/lib/gpr/arena.h, line 49 at r3 (raw file):

I think this is a more potentially involved change so I'd like to not handle that in this PR.

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

I have converted the remaining callers to use grpc_core::Arena.

I have added a TODO saying the transitional/vestigial gpr_arena API will be going away. Plan is to have a very quick PR next week that removes it. (I'd rather do that instead of a cherry-pick).

Thanks for doing this! Just a few remaining comments, all fairly minor.

Reviewed 7 of 8 files at r6, 18 of 18 files at r7.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, @soheilhy, and @vjpai)

src/core/ext/filters/client_channel/client_channel.cc, line 618 at r7 (raw file):

  gpr_timespec call_start_time_;
  grpc_millis deadline_;
  gpr_arena* arena_;

This should be changed to Arena.

src/core/ext/transport/chttp2/transport/incoming_metadata.h, line 27 at r7 (raw file):

struct grpc_chttp2_incoming_metadata_buffer {
  grpc_chttp2_incoming_metadata_buffer(grpc_core::Arena* arena) : arena(arena) {

Unrelated to this PR, but single-argument ctors should be marked explicit, as per the style guide.

src/core/lib/gpr/arena.h, line 49 at r3 (raw file):

Okay. In that case, please move the TODO about this from the .cc file to this file, right above the New() method.

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

In order to not need internal changes when this gets imported, don't the C API declarations need to go in the original header file name (in gpr, not gprpp)?

Reviewable status: 27 of 43 files reviewed, 7 unresolved discussions (waiting on @apolcyn, @AspirinSJL, @markdroth, @soheilhy, and @vjpai)

src/core/ext/filters/client_channel/client_channel.cc, line 618 at r7 (raw file):

Done.

src/core/ext/transport/chttp2/transport/incoming_metadata.h, line 27 at r7 (raw file):

Done.

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

Done.

Reviewable status: 27 of 43 files reviewed, 7 unresolved discussions (waiting on @apolcyn, @AspirinSJL, @markdroth, and @vjpai)

src/core/lib/gpr/arena.h, line 49 at r3 (raw file):

Done.

Reviewed 16 of 16 files at r8.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, and @vjpai)

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

The C API should now be deleted from this file.

src/core/lib/gprpp/arena.h, line 65 at r8 (raw file):

  // arena API to C++, we should consider replacing gpr_arena_alloc() with a
  // template that takes the type of the value being allocated, which
  // would allow us to use the alignment actually needed by the caller.

Please replace this text:

  As part of converting the
  arena API to C++, we should consider replacing gpr_arena_alloc() with a
  template that takes the type of the value being allocated, which
  would allow us to use the alignment actually needed by the caller.

with this:

When we have time, we should change this to instead use the alignment
of the type being allocated by this method.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @apolcyn, @AspirinSJL, @markdroth, and @vjpai)

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

Unable; they're inlined and need to be here as a result.

The gpr/arena.h has the declarations; the gprpp/arena.h file has the inlined definitions.

src/core/lib/gprpp/arena.h, line 65 at r8 (raw file):

Done.

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, and @vjpai)

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

Why not just put the inlined definitions in gpr/arena.h? It can include this file. (Yes, including something in gprpp from something in gpr is a little messy, but this code won't be around very long, so it's fine as a short-term measure.)

Reviewable status: 0 of 47 files reviewed, 4 unresolved discussions (waiting on @apolcyn, @AspirinSJL, @markdroth, and @vjpai)

src/core/lib/gpr/arena.h, line 85 at r3 (raw file):

Done.

Looks great!

Reviewed 42 of 44 files at r11.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @apolcyn, @AspirinSJL, and @vjpai)