Faster Sexp#line options

[presidentbeef]

Right now, Sexp#line can be used to either fetch or set the line number. This has led to some bugs which were addressed by ce28448

But Sexp#line is quite a bit slower (relatively) due to the added logic. The method gets called a lot, so it might make sense to have a faster path.

Benchmark code

require 'sexp_processor'
require 'benchmark/ips'

class Sexp
  def set_line x
    @line = x
    self
  end

  def get_line
    @line
  end
end

test_sexp = Sexp.new.line(1)

Benchmark.ips do |x|
  x.report "line" do
    test_sexp.line(10)
  end

  x.report "set_line" do
    test_sexp.set_line(10)
  end

  x.report "line=" do
    test_sexp.line = 10
  end

  x.compare!
end

Benchmark.ips do |x|
  x.report "line" do
    test_sexp.line
  end

  x.report "get_line" do
    test_sexp.get_line
  end

  x.compare!
end

Benchmark results

Warming up --------------------------------------
                line    88.816k i/100ms
            set_line   176.887k i/100ms
               line=   193.423k i/100ms
Calculating -------------------------------------
                line      1.708M (± 3.0%) i/s -      8.615M in   5.047446s
            set_line      6.326M (± 4.3%) i/s -     31.663M in   5.014701s
               line=      7.188M (± 3.2%) i/s -     35.977M in   5.010991s

Comparison:
               line=:  7187710.6 i/s
            set_line:  6326318.9 i/s - 1.14x  slower
                line:  1708412.4 i/s - 4.21x  slower

Warming up --------------------------------------
                line   145.990k i/100ms
            get_line   197.802k i/100ms
Calculating -------------------------------------
                line      3.323M (± 3.3%) i/s -     16.643M in   5.014754s
            get_line      8.896M (± 4.2%) i/s -     44.505M in   5.012103s

Comparison:
            get_line:  8896218.1 i/s
                line:  3322652.5 i/s - 2.68x  slower

I did a rough search-and-replace of calls to Sexp#line in SexpProcessor and RubyParser. The performance difference is maybe 3-5%. So maybe not worth it?

The method names could certainly use work. I like Sexp#with_line to suggest that it is returning self. Sexp#lineno or Sexp#line_no could be used for fetching? Since lines are often copied from existing Sexps, maybe a method like Sexp#copy_line to pass in a Sexp?

Just thoughts, not attached to any of this.

[zenspider]

The logic causes some slowdown, but a bigger chunk than I thought was happening because of that stupid n != UNASSIGNED, surprisingly... It's a plain object, so I expected it to be object identity comparison only. Not only was I wrong, but == is more costly than != these days???

So, line3 is the same as the current version, with the logic inverted to only use == for roughly an 11% boost. And line2 switches from == to .equal? for a 126%(!!) boost above that.

I will certainly do the latter... I'm not sure about adding get_ and set_... that's awfully clunky. I kinda want to revamp the whole library tbh.

Comparison:
               line=: 14376054.6 i/s
            set_line: 13954309.9 i/s - same-ish: difference falls within error
               line2:  8292654.0 i/s - 1.73x  slower # use equal?
               line3:  3681193.9 i/s - 3.91x  slower # cost of not: switching != to == ?!?!
                line:  3309975.9 i/s - 4.34x  slower # current

[zenspider]

Comparison:
               line=: 14512618.8 i/s
            set_line: 13018852.3 i/s - 1.11x  slower
          line_equal:  8181429.4 i/s - 1.77x  slower
            line_eq2:  3764150.4 i/s - 3.86x  slower
        line_current:  3473887.8 i/s - 4.18x  slower

[zenspider]

This is so fucking strange... using equal? is SLOWER for the getter case?!?! I have no fucking idea what's going on.

[presidentbeef]

I wonder if Nil#equal? vs. Object#equal? makes any difference?? So wild.

I mean, it's pretty fast as-is, this is really micro-optimizing, but I do find Brakeman spends a considerable amount of time setting line numbers if I use a sampling profiler.

[zenspider]

My how machines and ruby versions have changed...

Comparison:
            line_eq2: 10859680.5 i/s
           line_neq2: 10844208.1 i/s - same-ish: difference falls within error
         line_equal2: 10798477.8 i/s - 1.01x  (± 0.00) slower
          line_equal: 10667866.7 i/s - 1.02x  (± 0.00) slower
           orig_line: 10623854.1 i/s - 1.02x  (± 0.00) slower
            line_neq: 10608638.2 i/s - 1.02x  (± 0.00) slower

Comparison (UNASSIGNED frozen):
            line_eq2: 11025878.0 i/s
           line_neq2: 11020092.2 i/s - same-ish: difference falls within error
           orig_line: 10865724.2 i/s - 1.01x  (± 0.00) slower
            line_neq: 10795490.9 i/s - same-ish: difference falls within error
         line_equal2: 10355374.7 i/s - 1.06x  (± 0.00) slower
          line_equal: 10187097.6 i/s - 1.08x  (± 0.00) slower

[zenspider]

Out of curiosity, I made sneaky_line which is the line = nil version.

  def sneaky_line n = nil
    if n then
      set_line n
    else
      @line ||= nil
    end
  end

and it's 10% faster. Do you remember what the bugs were? Might be worth revisiting.

[presidentbeef]

Do you mean

This has led to some bugs...

If so, the issue was code like

sexp1.line(sexp2.line)

if sexp2.line is nil... then line returns nil instead of sexp1/self.

[zenspider]

What do you think about going back to the old version but provide a defensive one via en env var or something?

I'm benchmarking the shit out of conditional and equality combinations...

I'm currently leaning towards defaulting the arg to false instead of nil or some arbitrary object. False will get, non-integer will raise, otherwise set. That way nils will still be caught and handled. How's that sound?

[presidentbeef]

That works for me!

Edit: To be clear, I mean the false default works for me.

[zenspider]

OK. After a lot of benchmarking I've discovered a few things. The most costly bit out of all of this is the optional argument. The comparisons against that cost isn't zero, but is effectively noise compared to the cost of setting up the call frame and evaluating the optional argument when called with no args.

That is to say, if we want to squeeze all the performance from this that we can, we should move towards an explicit setter and getter. Combining them looked nice, but comes with a cost. To be fair, the cost isn't that much.

These are all roughly equal in cost:

  def line_orig n = UNASSIGNED
    if n != UNASSIGNED then
      raise ArgumentError, "setting %p.line %p" % [self, n] unless Integer === n
      @line = n
      self
    else
      @line ||= nil
    end
  end

  def line_false1 n = false
    raise ArgumentError, "setting %p.line %p" % [self, n] unless
      n == false or Integer === n
    if n then
      @line = n
      self
    else
      @line ||= nil
    end
  end

  def line_false2 n = false
    if n then
      raise ArgumentError, "setting %p.line %p" % [self, n] unless Integer === n
      @line = n
      self
    else
      raise ArgumentError, "setting %p.line %p" % [self, n] unless n == false
      @line ||= nil
    end
  end

Comparison:
     line_false1 get: 10745080.8 i/s
     line_false2 get: 10629262.5 i/s - 1.01x  (± 0.00) slower
       line_orig get: 10554966.4 i/s - same-ish: difference falls within error

and it is only when we really start to change the design that we start to get improvements:

  def line_false3 n = false
    if n then
      @line = n
      self
    else
      @line ||= nil
    end
  end

  def line_false4
    @line ||= nil
  end

Comparison:
     line_false4 get: 14605269.0 i/s
     line_false3 get: 11613797.3 i/s - 1.26x  (± 0.00) slower
     line_false1 get: 10723789.5 i/s - 1.36x  (± 0.00) slower
     line_false2 get: 10645948.2 i/s - 1.37x  (± 0.00) slower
       line_orig get: 10468116.0 i/s - 1.40x  (± 0.00) slower

So, at this point, I'm tempted to just leave it be and start planning a migration to an explicit setter with a deprecation warning in the old line if an argument is provided.

Your old benchmark run on 3.1 (with a minor modification to allow for comparing attr_reader vs my original line:

ruby 3.1.0p0 (2021-12-25 revision fb4df44d16) [arm64-darwin21]

Comparison:
               line=: 14272066.9 i/s
            set_line: 13295822.8 i/s - 1.07x  (± 0.00) slower
           line_orig:  5268387.4 i/s - 2.71x  (± 0.00) slower

Comparison:
          attr  line: 17919612.9 i/s
            get_line: 15669000.6 i/s - 1.14x  (± 0.00) slower
           line_orig: 11112854.8 i/s - 1.61x  (± 0.00) slower