Skip to content

Commit

Permalink
Remove useless Rule::Normal#initialize (#128)
Browse files Browse the repository at this point in the history
I noticed that in our app `PublicSuffix` was taking 180ms to load the suffixes.

After some profiling:

```ruby
==================================
  Mode: cpu(1000)
  Samples: 70 (0.00% miss rate)
  GC: 17 (24.29%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
        40  (57.1%)          20  (28.6%)     #<Module:0x007fd68f8b8860>.factory
        20  (28.6%)          17  (24.3%)     PublicSuffix::Rule::Normal#initialize
         6   (8.6%)           6   (8.6%)     PublicSuffix::Domain.name_to_labels
        11  (15.7%)           5   (7.1%)     block in PublicSuffix::List#reindex!
         3   (4.3%)           3   (4.3%)     PublicSuffix::Rule::Base#initialize
         9  (12.9%)           2   (2.9%)     PublicSuffix::List#add
        53  (75.7%)           0   (0.0%)     <main>
        53  (75.7%)           0   (0.0%)     <main>
        53  (75.7%)           0   (0.0%)     block in <main>
        46  (65.7%)           0   (0.0%)     PublicSuffix::List.parse
        46  (65.7%)           0   (0.0%)     PublicSuffix::List#initialize
        42  (60.0%)           0   (0.0%)     block in PublicSuffix::List.parse
        42  (60.0%)           0   (0.0%)     block (2 levels) in PublicSuffix::List.parse
        11  (15.7%)           0   (0.0%)     PublicSuffix::List#reindex!
         7  (10.0%)           0   (0.0%)     block (2 levels) in <main>

```

`PublicSuffix::Rule::Normal#initialize` happens to be a hotspot, which is weird since it does nothing more than forwarding the parameters.

After taking it out:

```ruby
==================================
  Mode: cpu(1000)
  Samples: 58 (0.00% miss rate)
  GC: 22 (37.93%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
        20  (34.5%)          19  (32.8%)     #<Module:0x007f7fb8899380>.factory
         9  (15.5%)           9  (15.5%)     PublicSuffix::Domain.name_to_labels
        16  (27.6%)           7  (12.1%)     block in PublicSuffix::List#reindex!
         1   (1.7%)           1   (1.7%)     PublicSuffix::Rule::Base#initialize
        36  (62.1%)           0   (0.0%)     <main>
        36  (62.1%)           0   (0.0%)     block in <main>
        36  (62.1%)           0   (0.0%)     <main>
        29  (50.0%)           0   (0.0%)     PublicSuffix::List.parse
        29  (50.0%)           0   (0.0%)     PublicSuffix::List#initialize
        20  (34.5%)           0   (0.0%)     block in PublicSuffix::List.parse
        16  (27.6%)           0   (0.0%)     PublicSuffix::List#reindex!
        20  (34.5%)           0   (0.0%)     block (2 levels) in PublicSuffix::List.parse
         7  (12.1%)           0   (0.0%)     PublicSuffix::List#add
         7  (12.1%)           0   (0.0%)     block (2 levels) in <main>
```

I also benchmarked: `PublicSuffix::List.parse(source, private_domains: false)`:

```ruby
require 'public_suffix'
require 'benchmark/ips'

Benchmark.ips do |x|
  x.report('parse') do
    PublicSuffix::List.parse(source, private_domains: false)
  end
end
```

Before:

```
Warming up --------------------------------------
               parse     1.000  i/100ms
Calculating -------------------------------------
               parse     16.780  (± 6.0%) i/s -     84.000  in   5.020938s
```

After:

```
Warming up --------------------------------------
               parse     2.000  i/100ms
Calculating -------------------------------------
               parse     22.670  (± 4.4%) i/s -    114.000  in   5.044953s
```


So clearly the `**options; super(**options)` is quite detrimental for performances.

So I decided to replace that pattern with explicitly forwarding of the named arguments, and got the following numbers:

```
Warming up --------------------------------------
               parse     3.000  i/100ms
Calculating -------------------------------------
               parse     38.982  (± 2.6%) i/s -    390.000  in  10.015638s
```

So I totally agree that `**options` is a nicer way to delegate, but given it's performance cost, I think it's better not to use it in hotspots.
  • Loading branch information
casperisfine authored and weppos committed Jan 2, 2017
1 parent 8312db0 commit cd7bf46
Showing 1 changed file with 6 additions and 13 deletions.
19 changes: 6 additions & 13 deletions lib/public_suffix/rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,6 @@ def decompose(*)
# Normal represents a standard rule (e.g. com).
class Normal < Base

# Initializes a new rule from +definition+.
#
# @param definition [String] the rule as defined in the PSL
def initialize(definition, **options)
super(definition, **options)
end

# Gets the original rule definition.
#
# @return [String] The rule definition.
Expand Down Expand Up @@ -226,8 +219,8 @@ class Wildcard < Base
# for each wildcard rule.
#
# @param definition [String] the rule as defined in the PSL
def initialize(definition, **options)
super(definition.to_s[2..-1], **options)
def initialize(definition, private: false)
super(definition.to_s[2..-1], private: private)
end

# Gets the original rule definition.
Expand Down Expand Up @@ -275,8 +268,8 @@ class Exception < Base
# for each wildcard rule.
#
# @param definition [String] the rule as defined in the PSL
def initialize(definition, **options)
super(definition.to_s[1..-1], **options)
def initialize(definition, private: false)
super(definition.to_s[1..-1], private: private)
end

# Gets the original rule definition.
Expand Down Expand Up @@ -338,15 +331,15 @@ def length
#
# @param [String] content The rule content.
# @return [PublicSuffix::Rule::*] A rule instance.
def self.factory(content, **options)
def self.factory(content, private: false)
case content.to_s[0, 1]
when STAR
Wildcard
when BANG
Exception
else
Normal
end.new(content, **options)
end.new(content, private: private)
end

# The default rule to use if no rule match.
Expand Down

0 comments on commit cd7bf46

Please sign in to comment.