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

Improve CachedAssetFile #118

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Conversation

stevendaniels
Copy link
Contributor

@stevendaniels stevendaniels commented Mar 19, 2020

Closes #108

Some speed improvements for the CachedAssetFile class. Doing the key length comparisons once and removing the generated regexes drastically sped up the implementation.

Here are the benchmarks

Benchmark.ips do |x|
  x.report("original_cached")    { cached.named(svgs.sample) }
  x.report("noncached") { noncached.named(svgs.sample) }
  x.report("new_cached") { supercached.named(svgs.sample) }

  x.compare!
end
Warming up --------------------------------------
     original_cached    35.000  i/100ms
           noncached    82.000  i/100ms
          new_cached     3.024k i/100ms
Calculating -------------------------------------
     original_cached    383.179  (±15.9%) i/s -      1.890k in   5.088049s
           noncached      1.041k (±18.2%) i/s -      5.002k in   5.022027s
          new_cached     30.145k (±15.6%) i/s -    145.152k in   5.043947s

Comparison:
          new_cached:    30145.5 i/s
           noncached:     1040.6 i/s - 28.97x  slower
     original_cached:      383.2 i/s - 78.67x  slower

This isn't a perfect benchmark (it's using .sample), but the cache is clearly faster in the new implementation.

@stevendaniels
Copy link
Contributor Author

stevendaniels commented Mar 19, 2020

Using include? instead of end_with? fixes the broken test, but it slightly slower. It's still faster than un-cached.

Warming up --------------------------------------
          old_cached    35.000  i/100ms
           noncached   104.000  i/100ms
          new_cached     1.213k i/100ms
Calculating -------------------------------------
              cached    394.127  (±14.0%) i/s -      1.925k in   5.012903s
           noncached      1.166k (±17.6%) i/s -      5.616k in   5.005705s
          new_cached     15.572k (±13.3%) i/s -     75.206k in   5.062016s

Comparison:
         new_cached:    15571.8 i/s
          noncached:     1165.9 i/s - 13.36x  slower
             cached:      394.1 i/s - 39.51x  slower

Closes jamesmartin#108

Some speed improvements for the CachedAssetFile class. Doing the key length comparisons once and removing the generated regexes drastically sped up the implementation.

Here are the benchmarks

```ruby
Benchmark.ips do |x|
  x.report("original_cached")    { cached.named(svgs.sample) }
  x.report("noncached") { noncached.named(svgs.sample) }
  x.report("new_cached") { supercached.named(svgs.sample) }

  x.compare!
end
```
```
Warming up --------------------------------------
          old_cached    35.000  i/100ms
           noncached   104.000  i/100ms
          new_cached     1.213k i/100ms
Calculating -------------------------------------
              cached    394.127  (±14.0%) i/s -      1.925k in   5.012903s
           noncached      1.166k (±17.6%) i/s -      5.616k in   5.005705s
          new_cached     15.572k (±13.3%) i/s -     75.206k in   5.062016s

Comparison:
         new_cached:    15571.8 i/s
          noncached:     1165.9 i/s - 13.36x  slower
             cached:      394.1 i/s - 39.51x  slower
```
@jamesmartin
Copy link
Owner

Thanks for opening this pull request, @stevendaniels. This looks like a worthwhile improvement! ✨

@jamesmartin jamesmartin merged commit 07809a7 into jamesmartin:master Mar 23, 2020
@stevendaniels stevendaniels deleted the patch-1 branch March 26, 2020 23:31
@davefp davefp mentioned this pull request Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cached assets seems to take longer to load than when reading from file
2 participants