shoes / shoes4

Shoes 4 : the next version of Shoes
Other
1.59k stars 194 forks source link

Move RenamedDelegate to proper locations and document it #1550

Closed jasonrclark closed 6 years ago

jasonrclark commented 6 years ago

As part of the big #620 documentation push, noticed this class was declared outside the Shoes module so moving it to its own place. Wrote brief description for it.

PragTob commented 6 years ago

I wouldn't necessarily move it inside the Shoes namespace... it has nothing directly to do with shoes, it's a common utility. My plan was always to extract it into a micro library, but not sure how many people would use it though (I never found myself wanting to use it again) - might be nice though.

jasonrclark commented 6 years ago

So I moved it for two reasons:

1) Alphabetically it's showing at the top of the entire list, which is awkward looking on the site (image below) 2) The current namespacing doesn't align with where the file lives... if we did want to leave it outside Shoes, I'd rather pop the file up to shoes-core/lib instead of shoes-core/lib/shoes

image

In terms of reuse, it appears we only use it on the Shoes::Dimensions class even within Shoes. While it's certainly tidy, I started wondering a little about the performance we've seen before with metaprogrammed methods in our hot paths.

Whipped up this benchmark:

# frozen_string_literal: true

require 'benchmark/ips'

class DirectDimensions < Shoes::Dimensions
  def absolute_left
    @x_dimension.absolute_start
  end

  def element_left
    @x_dimension.element_start
  end

  def margin_left
    @x_dimension.margin_start
  end
end

Benchmark.ips do |benchmark|
  original = Shoes::Dimensions.new(nil, 0, 0, 100, 100, margin: 10)
  direct = DirectDimensions.new(nil, 0, 0, 100, 100, margin: 10)

  benchmark.report 'original' do
    original.absolute_left
    original.element_left
    original.margin_left
  end

  benchmark.report 'direct' do
    direct.absolute_left
    direct.element_left
    direct.margin_left
  end

  benchmark.compare!
end

Which had this result:

Warming up --------------------------------------
            original    10.813k i/100ms
              direct    20.696k i/100ms
Calculating -------------------------------------
            original    142.329k (± 3.5%) i/s -    713.658k in   5.020571s
              direct    330.839k (± 3.0%) i/s -      1.656M in   5.009206s

Comparison:
              direct:   330839.0 i/s
            original:   142329.4 i/s - 2.32x  slower

From what I've seen before when making our low-level stuff faster making a noticable difference in the UI layout (and hence the snappiness of Shoes apps) maybe we should consider moving away from this class altogether? Thoughts @PragTob?

PragTob commented 6 years ago

Good thinking I think 🎉

We could make renamed delegate faster... but in the end we use it in one place and we'd remove some magic with a more simple and seemingly faster method of access.

I'm confused why that is though. I thought if anything def_delegator would be faster than a normal hand rolled method definition so this confuses me... apparently I'm wrong and it uses define_method or something and catches scope along with it.

Anyhow, yeah definitely - don't think I'd write it again like this today - problem is not big enough to pull out the big metaprogramming guns I think :)

jasonrclark commented 6 years ago

Yeah exactly why there’s a speed diff is still hazy to me but seems like a plan.