Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Favoring explicit class declaration #17042

Merged
merged 1 commit into from Mar 29, 2022
Merged

Conversation

jeremyf
Copy link
Contributor

@jeremyf jeremyf commented Mar 29, 2022

What type of PR is this? (check all applicable)

  • Refactor

Description

In helping track down #17041 I was looking at our cache
busting logic. We were looking up a constant via the .const_get call
from a string already defined inline. This refactor short-circuits
naming a string, capitalizing it, then looking in the class's registered
constants.

There's also a subtle bug in the original; When provider equals
"another_cache", the #capitalize method would return
"Another_cache", whereas #classify will return "AnotherCache".

Below are some benchmarks for the change.

require "benchmark"

module EdgeCache
  class Bust
    def by_class
      Fastly
    end

    def by_const_get
      self.class.const_get("fastly".classify)
    end
  end
end

Benchmark.bmbm do |x|
  x.report("by_class") do
    1000.times do
      EdgeCache::Bust.new.by_class
    end
  end
  x.report("by_const_get") do
    1000.times do
      EdgeCache::Bust.new.by_const_get
    end
  end
end
> bin/rails runner /Users/jfriesen/git/forem/bench.rb

Rehearsal ------------------------------------------------
by_class       0.001239   0.000186   0.001425 (  0.001342)
by_const_get   0.011398   0.000136   0.011534 (  0.011538)
--------------------------------------- total: 0.012959sec

                   user     system      total        real
by_class       0.000973   0.000030   0.001003 (  0.000969)
by_const_get   0.011102   0.000032   0.011134 (  0.011104)

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

None, this is already tested.

UI accessibility concerns?

None.

Added/updated tests?

  • No, and this is why:

[Forem core team only] How will this change be communicated?

  • I will share this change internally with the appropriate teams

In helping track down #17041 I was looking at our cache
busting logic.  We were looking up a constant via the `.const_get` call
from a string already defined inline.  This refactor short-circuits
naming a string, capitalizing it, then looking in the class's registered
constants.

There's also a subtle bug in the original; When `provider` equals
"another_cache", the `#capitalize` method would return
`"Another_cache"`, whereas `#classify` will return `"AnotherCache"`.

Below are some benchmarks for the change.

```ruby
require "benchmark"

module EdgeCache
  class Bust
    def by_class
      Fastly
    end

    def by_const_get
      self.class.const_get("fastly".classify)
    end
  end
end

Benchmark.bmbm do |x|
  x.report("by_class") do
    1000.times do
      EdgeCache::Bust.new.by_class
    end
  end
  x.report("by_const_get") do
    1000.times do
      EdgeCache::Bust.new.by_const_get
    end
  end
end
```

```shell
> bin/rails runner /Users/jfriesen/git/forem/bench.rb

Rehearsal ------------------------------------------------
by_class       0.001239   0.000186   0.001425 (  0.001342)
by_const_get   0.011398   0.000136   0.011534 (  0.011538)
--------------------------------------- total: 0.012959sec

                   user     system      total        real
by_class       0.000973   0.000030   0.001003 (  0.000969)
by_const_get   0.011102   0.000032   0.011134 (  0.011104)
```
@jeremyf jeremyf requested a review from a team March 29, 2022 03:09
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Mar 29, 2022
@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Mar 29, 2022
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Mar 29, 2022
@jeremyf jeremyf merged commit 6e81773 into main Mar 29, 2022
@jeremyf jeremyf deleted the jeremyf/removing-constant-lookup branch March 29, 2022 17:52
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants