Remove GC::Profiler.enable

[tombruijn]

I don't think we should provide this in our gem, but have users add this
to their own app manually in a Rails's application.rb file, or
development.rb.

This way they also have more control over when to disable it again, by
using GC::Profiler.disable if they only want to enable it for certain
parts of their app. It leaves control with the app developer.

The enable_gc_instrumentation config option should remain so that we
fetch the Garbage Collection profiler wrapper or the nil wrapper
NilProfiler.

Part of #868
[skip changeset] because this is undocumented behavior

[unflxw]

The enable_gc_instrumentation config option should remain so that we fetch the Garbage Collection profiler wrapper or the nil wrapper NilProfiler.

Given the changes in #877 to make the GC time gauge call GC::Profiler.enabled? to decide whether to report the value, is there still value in having the NilProfiler and the config option around? I can't imagine calling GC::Profiler.enabled? once per minute is too expensive.

If we're going to ask users to manually call GC::Profiler.enable to turn on the profiler, which I think is the right decision, then the enable_gc_instrumentation config option seems redundant to me.

[tombruijn]

@unflxw Great you asked! I'm not sure. Is there a scenario where one would want to enable GC profiling, but not report it to AppSignal when it's enabled? That's the only purpose enable_gc_instrumentation would have after these changes.

Another thing to consider is: we have GC profiling per transaction, tracking the GC time at the start and finish of a transaction. This happens more often than once a minute. I left it in right now with the config option, until we can discuss what to do with Transaction GC profiling on Monday. And if it's ready to be shipped. I put it on the agenda.

Removing enable_gc_instrumentation can be another PR if we see that impact of calling GC::Profiler.enabled? that often is negligible. Right now it's more steps to enable GC profiling in AppSignal but doesn't accidentally make everything slower in the AppSignal gem.

[tombruijn]

Quick benchmark how fast GC::Profiler.enabled? is:

› ruby bench.rb
Warming up --------------------------------------
GC::Profiler.enabled?
                         1.997M i/100ms
Appsignal.config[:enable_gc_instrumentation]
                         1.173M i/100ms
Calculating -------------------------------------
GC::Profiler.enabled?
                         19.978M (± 0.3%) i/s -    101.843M in   5.097836s
Appsignal.config[:enable_gc_instrumentation]
                         11.763M (± 0.2%) i/s -     59.831M in   5.086207s

Comparison:
GC::Profiler.enabled?: 19977722.0 i/s
Appsignal.config[:enable_gc_instrumentation]: 11763470.4 i/s - 1.70x  (± 0.00) slower

Script:

# bench.rb
require "benchmark/ips"

class Appsignal
  def self.config
    @config ||= Config.new
  end

  class Config
    def initialize
      @options ||= { :enable_gc_instrumentation => false }
    end

    def [](key)
      @options[key]
    end
  end
end

GC::Profiler.enable # Enabled/disabled does not affect the benchmark

Benchmark.ips do |x|
  # Configure the number of seconds used during
  # the warmup phase (default 2) and calculation phase (default 5)
  x.config(:time => 5, :warmup => 2)

  x.report("GC::Profiler.enabled?") { GC::Profiler.enabled? }

  x.report("Appsignal.config[:enable_gc_instrumentation]") do
    Appsignal.config[:enable_gc_instrumentation]
  end

  # Compare the iterations per second of the various reports!
  x.compare!
end

[unflxw]

Cool that you did the benchmark! Looking at the implementation (I love that these Ruby documentation pages link you to the source code in the interpreter) GC::Profiler.enabled? seems to be about as cheap as reading a boolean can be.

[tombruijn]

That's great! Then that's not a blocker to only use GC::Profiler.enabled? later.

For this PR I'd like to merge it as is until we decide what to do with the enable_gc_instrumentation config option on Monday. Agreed?

[unflxw]

EDIT: Sorry, didn't read your comment before posting this one! Doing it later is also fine with me. 👍

Given that reading our flag is slower (and the added indirection of our NilProfiler wrapper probably adds to that) I'd go with getting rid of the NilProfiler and the enable_gc_instrumentation. The core idea of this change is that we want to allow you to enable and disable the profiler in the Ruby-idiomatic way, instead of having to do it through our config flag. I'd say, let's commit to that line of thought and just check GC::Profiler.enabled? whenever we want to maybe-profile something.

[tombruijn]

Thanks! I agree with what you're saying, but let's first run it by Thijs first about the Transaction GC, because he made it way back when.
The implementation as you describe it is probably fine.

Edit: I'd actually still keep the NilProfiler so it doesn't have to do mutex hoops in GarbageCollection::Profiler when there is no GC profiling.