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

Cache regex in template OptimizedFileSystemResolver #35228

Closed

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Feb 12, 2019

This is a follow up to #33860, which made templates faster by doing more work in ruby to reduce I/O. This PR reduces the amount of work we're doing in ruby. Like the previous PR this will mostly benefit startup time and development mode (where Resolver.caching is false).

Previously we were building a regex for each call to #query (which is called for each view_path on each search). Instead, this PR creates a regex for just the details component of the path (which itself is slightly faster), and caches that regex.

A regex used to look like:

/\A\/Users\/jhawthorn\/src\/rails\/railties\/lib\/rails\/templates\/rails\/welcome\/index(\.(?<locale>en))?(\.(?<formats>html|text|js|css|ics|csv|vcf|vtt|png|jpeg|gif|bmp|tiff|svg|mpeg|mp3|ogg|m4a|webm|mp4|otf|ttf|woff|woff2|xml|rss|atom|yaml|multipart_form|url_encoded_form|json|pdf|zip|gzip))?(\+(?<variants>))?(\.(?<handlers>raw|erb|html|builder|ruby))?\z/

Now looks like:

/\A\(\.(?<locale>en))?(\.(?<formats>html|text|js|css|ics|csv|vcf|vtt|png|jpeg|gif|bmp|tiff|svg|mpeg|mp3|ogg|m4a|webm|mp4|otf|ttf|woff|woff2|xml|rss|atom|yaml|multipart_form|url_encoded_form|json|pdf|zip|gzip))?(\+(?<variants>))?(\.(?<handlers>raw|erb|html|builder|ruby))?\z/

Benchmark

The speedup depends a lot on how slow the globs are. Listing directories on my Linux machine is way faster than on my MacBook 🤔. On my MacBook it's still ~1.5x faster 🎉

Benchmark:

require "benchmark/ips"
require "action_view"
require "action_pack"
require "action_controller"

class TestController < ActionController::Base
end

ActionView::Resolver.caching = false
TestController.view_paths = [File.expand_path("./railties/lib/rails/templates")]
context = TestController.new.lookup_context

Benchmark.ips do |x|
  x.report("template") do
    context.find_template("rails/welcome/index", [], false, {}, {})
  end
end

Before:

Warming up --------------------------------------
            template   387.000  i/100ms
Calculating -------------------------------------
            template      3.821k (± 5.0%) i/s -     19.350k in   5.078706s

After:

Warming up --------------------------------------
            template   586.000  i/100ms
Calculating -------------------------------------
            template      5.717k (± 7.3%) i/s -     28.714k in   5.055056s

cc @tenderlove @eileencodes

If we find no candidate templates, we can return before building the
regex to compare details.
This makes the regex only match on the "details" component of the
filename.
Now that the regex doesn't include the path component, only the details
component, it makes sense to cache. The same details regex will be used
across many templates.

We already use details as one of the Resolver cache keys, so it should
be fine to cache on.
@rails-bot rails-bot bot added the actionview label Feb 12, 2019
@matthewd
Copy link
Member

Is it safe to use details directly as a cache key here? Nearby we have a separate details_key thingy.

Does it only contain entries for EXTENSIONS.keys? (And are they immutable?)

The biggest thing that I default to worrying about inside caches is reloading -- but as long as those four are the only things in there, that seems fine.

After that the next concern is user-supplied data: I think we might have a bigger issue with that one. Can I add an entry to this permanent cache by e.g. tacking ?format=foo onto an arbitrary URL?

@rails-bot rails-bot bot added the actionpack label Feb 19, 2019
It doesn't make sense that we're accepting these escape sequences from
the user.
@rafaelfranca rafaelfranca added this to the 6.0.0 milestone Feb 19, 2019
@jhawthorn
Copy link
Member Author

I think I'll take a different approach with this. I'm hoping to use the same regex for parsing regardless of details.

@jhawthorn jhawthorn closed this Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants