Always call MemUsage::Collect to collect metrics from a field

[danakj]

Previously Collect() was used for types that implemented CollectMemUsage() but otherwise Add() was used. This required the caller to think about the type of the field and know/decide which method to use.

Now, the caller always uses Collect() unless they are adding specific byte values, in which case Add is used. Typically then, Add will only be used to implement the CollectMemUsage() function.

To do this we require all Collect() methods to be templates so that they all be a single overload set. The Collect on BumpPtrAllocator is converted to a template that checks std::same_as<llvm::BumpPtrAllocator, T>.

[danakj]

I see there's different kinds of BumpPtrAllocators in llvm, I guess so far we've only used the default parameters. This requires clause would match them all, but it might be preferable to write this as:

  template <typename AllocatorT, size_t SlabSize, size_t SizeThreshold,
            size_t GrowthDelay>
  auto Collect(std::string label,
               const llvm::BumpPtrAllocatorImpl<
                   AllocatorT, SlabSize, SizeThreshold, GrowthDelay>& allocator)
      -> void {

OTOH this ties the function more to the llvm template type API, which may be more likely to break (though seems still unlikely) when rolling LLVM.

WDYT?

Edit: FWIW neither option matches SpecificBumpPtrAllocator, another overload would be needed for that.

[zygoloid]

I only found three uses of llvm::BumpPtrAllocatorImpl in total across all of LLVM and its subprojects (two of which are actually using llvm::BumpPtrAllocator just spelled in a more verbose way), so I think handling only the special case of llvm::BumpPtrAllocator here should be fine.

[danakj]

Ok, done with same_as. PTAL

[zygoloid]

Thanks, this looks like a nice improvement.

[zygoloid]

I only found three uses of llvm::BumpPtrAllocatorImpl in total across all of LLVM and its subprojects (two of which are actually using llvm::BumpPtrAllocator just spelled in a more verbose way), so I think handling only the special case of llvm::BumpPtrAllocator here should be fine.

[zygoloid]

The template / same_as machinery here is a bit unusual. Can we use a const BumpPtrAllocator& parameter instead? (The same_as approach will suppress implicit conversions to BumpPtrAllocator; if that's important I think it'd be useful for the comment on this function to explain that to future readers of the code.)

[danakj]

If it's not a template then we get ambiguous overloads between the non-template method and the requires method at every callsite:

toolchain/sem_ir/file.cpp:168:13: error: call to member function 'Collect' is ambiguous
  mem_usage.Collect(MemUsage::ConcatLabel(label, "name_scopes_"), name_scopes_);
  ~~~~~~~~~~^~~~~~~
./toolchain/base/mem_usage.h:52:8: note: candidate function
  auto Collect(std::string label, const llvm::BumpPtrAllocator& allocator) -> void {
       ^
./toolchain/base/mem_usage.h:99:8: note: candidate function [with T = Carbon::SemIR::NameScopeStore]
  auto Collect(llvm::StringRef label, const T& arg) -> void {
       ^

[jonmeow]

Maybe zygoloid will have a better suggestion, but I think C++ is just really unhappy with the std::string + llvm::StringRef aspect of the overload. So to note one option here:

1. Change the signature of CollectMemUsage to replace llvm::StringRef label with const std::string& label

2. Change the relevant functions here to:

  // Adds memory usage for an object that provides `CollectMemUsage`.
  //
  // The expected signature of `CollectMemUsage` is above, in MemUsage class
  // comments.
  template <typename T>
  auto Collect(const std::string& label, const T& arg) -> void {
    arg.CollectMemUsage(*this, label);
  }

  // Adds memory usage for a `llvm::BumpPtrAllocator`.
  auto Collect(std::string label, const llvm::BumpPtrAllocator& allocator)
      -> void {
    Add(std::move(label), allocator.getBytesAllocated(),
        allocator.getTotalMemory());
  }

Due to how the labels are handled, I think string would be fine.

[danakj]

Oh yeah you're right, thanks for pointing that out. I didn't understand why it required a template, but it's the string vs StringRef (I still don't fully get why that causes this situation tbqh).

There is only a single place where CollectMemUsage was not already constructing a new string from its input to pass to Collect():

carbon-lang/toolchain/sem_ir/name_scope.h

Lines 170 to 173 in 534100e

	auto CollectMemUsage(MemUsage& mem_usage, llvm::StringRef label) const
	-> void {
	mem_usage.Collect(label, values_);
	}

That suggests that yeah, using a std::string is probably fine.

[danakj]

I updated the PR to drop the same_as template for the BumpPtrAllocator overload. I changed the generic Collect(label, const T&) to receive a std::string.

• I did not change the signature of the CollectMemUsage methods, and the requires clause expects them to receive llvm::StringRef. While they could receive const std::string& instead, as we have a std::string at the caller, by using llvm::StringRef it forces an explicit conversion to a newly allocated std::string in the Collect() call instead of an implicit copy from const std::string& which seems generally better to me, but I am not familiar with the expectations/practices here of using view types vs const-reference-to-owning-type. There is a single place (name_scope.h mentioned above) that needed to do this explicit string copy when it forwards through the label.
• I also did not change the signature of Collect to take const std::string& as that would introduce string copies in every call to them where right now only a single Collect call requires a copy.

[danakj]

There is another option here though, which is to template the label parameter as well on the general Collect method:

  template <typename S, typename T>
    requires std::convertible_to<S, llvm::StringRef> &&
             requires(MemUsage& mem_usage, llvm::StringRef label,
                      const T& arg) {
               { arg.CollectMemUsage(mem_usage, label) };
             }
  auto Collect(const S& label, const T& arg) -> void {
    arg.CollectMemUsage(*this, label);
  }

This eliminates the string copy in name_scope.h, as it can pass through the llvm::StringRef directly. In the rest of cases where a new string was constructed, it's converted to llvm::StringRef inside the body of Convert() instead of at the place of argument construction. Maybe that's the best option?

[zygoloid]

Oh, wow. So there were two things going wrong here:

1. The big one: BumpPtrAllocator has a broken unconstrained constructor that will implicitly convert from anything. It does this to support constructing custom inner allocators. There are zero in-tree uses of custom allocators for BumpPtrAllocator in LLVM (outside the tests). That makes the BumpPtrAllocator overload viable for any argument.
2. The small one: the difference in string parameter types leads to an ambiguity between an exact match for the second parameter and an exact match for the first parameter for all cases where the second argument is not a BumpPtrAllocator.

I think we should make all the overloads have the same first parameter type (as this PR now does, thanks!) on our side, and we should also look at (at least) making the BumpPtrAllocator converting constructor explicit (and maybe also constraining it to only accept argument types that can be used to construct the inner allocator) on the LLVM side.

[zygoloid]

(If we care about the potential extra allocations for the strings, we could use llvm::Twine instead of std::string, but I guess we're usually staying within SSO anyway?)