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

Use faster globs for template resolving #33860

Merged
merged 2 commits into from Sep 13, 2018

Conversation

Projects
None yet
5 participants
@jhawthorn
Contributor

jhawthorn commented Sep 13, 2018

Fixes #20752

ActionView::PathResolver (including the more optimized subclasses) works by evaluating a large glob pattern.

Its search is mostly equivalent to:

Dir["app/views/users/show{.en,}{.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,}{}{.raw,.erb,.html,.builder,.ruby,}"]

This is relatively slow. Ruby will check the existance of each possible file:
First app/views/users/show.en.html.raw,
then app/views/users/show.en.html.erb,
then app/views/users/show.en.html.html, ....etc

For the example above, which is not the worst - variants and fallback locales would add to complexity, that's 2 * 35 * 6 = 420 checks. Not nice.

Each of these checks needs 4 syscalls (it checks each path up the tree), resulting in 1682 syscalls. This would be even worse in a real app because it scans an absolute path.

It's much faster, as proposed in #20752 to scan the directory with a wildcard, and then filter the results in Ruby.

Dir["app/views/users/show*"]

This is only 14 syscalls. Great!


Filtering this would be trivial using the existing glob and File.fnmatch, but the problem we then run into is sorting (there is some discussion of this in #20752).

Although Ruby's docs don't guarantee an order returned by Dir.glob, it is consistent, and Rails relies on that order (ex. we want to prefer show.en.html.erb to show.html.erb).

This PR solves this by also building a regex to match paths and then using it to both ensure each file actually matches (vs just shares the prefix) and extract the details values which were matched. We then use the matched values to sort the results into the same order they would have been returned by the original glob pattern.

Benchmark

require './config/environment'

Benchmark.ips do |x|
  x.report "lookup users/show" do
    ApplicationController.new.lookup_context.find_template("users/show", [], false, {}, {})
  end
end

Before

Warming up --------------------------------------
   lookup users/show     6.000  i/100ms
Calculating -------------------------------------
   lookup users/show     61.302  (± 4.9%) i/s -    306.000  in   5.011577s

After

Warming up --------------------------------------
   lookup users/show   353.000  i/100ms
Calculating -------------------------------------
   lookup users/show      3.845k (± 5.3%) i/s -     19.415k in   5.064962s

Thanks for reading ❤️

cc @eileencodes @brianmario @tenderlove

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 13, 2018

Member

Build is broken.

Member

rafaelfranca commented Sep 13, 2018

Build is broken.

@rafaelfranca rafaelfranca merged commit 7db3a6e into rails:master Sep 13, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Erowlin

This comment has been minimized.

Show comment
Hide comment
@Erowlin

Erowlin Sep 17, 2018

I'm not familiar with the release system on Rails. Checking the documentation, I can read this:

Changes that are merged into master are intended for the next major release of Rails.
Source

Will this be back-ported? Or it won't be for the next major, but rather a minor?

Edit: Great job @jhawthorn, thank you for your contribution!

Erowlin commented Sep 17, 2018

I'm not familiar with the release system on Rails. Checking the documentation, I can read this:

Changes that are merged into master are intended for the next major release of Rails.
Source

Will this be back-ported? Or it won't be for the next major, but rather a minor?

Edit: Great job @jhawthorn, thank you for your contribution!

@clemensp

This comment has been minimized.

Show comment
Hide comment
@clemensp

clemensp Oct 9, 2018

@jhawthorn :
Thanks for the fix. Addressed slowdowns my team was experiencing after extracting a bunch of modules with rails engines.
I'm eager to get this back-ported, and am willing to support the efforts to make it happen.

@rafaelfranca : Which versions of Rails would accept this back-port? I'm interested in getting this into 4.2, 5.0, and 5.1. I did some searching, but couldn't find the policy around back-porting.

Thanks!

clemensp commented Oct 9, 2018

@jhawthorn :
Thanks for the fix. Addressed slowdowns my team was experiencing after extracting a bunch of modules with rails engines.
I'm eager to get this back-ported, and am willing to support the efforts to make it happen.

@rafaelfranca : Which versions of Rails would accept this back-port? I'm interested in getting this into 4.2, 5.0, and 5.1. I did some searching, but couldn't find the policy around back-porting.

Thanks!

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Oct 9, 2018

Member

@clemens Check out the Maintenance Policy for what gets backported 👍

Member

jeremy commented Oct 9, 2018

@clemens Check out the Maintenance Policy for what gets backported 👍

@clemensp

This comment has been minimized.

Show comment
Hide comment
@clemensp

clemensp Oct 9, 2018

@jeremy : Thanks. I guess based on the Maintenance Policy, a performance optimization mainly for development mode would be categorized under "Bug Fixes" at best.

Is the Rails team interested in a PR for Rails 5.2?

Also, any chance a back-port for Rails 5.0 or 5.1 would be accepted?
My team's going through a long upgrade process to get to the latest Rails version, and it would be great to get the fix back-ported instead of having to maintain a custom back-port. I support whatever decision the Rails team makes. :)

clemensp commented Oct 9, 2018

@jeremy : Thanks. I guess based on the Maintenance Policy, a performance optimization mainly for development mode would be categorized under "Bug Fixes" at best.

Is the Rails team interested in a PR for Rails 5.2?

Also, any chance a back-port for Rails 5.0 or 5.1 would be accepted?
My team's going through a long upgrade process to get to the latest Rails version, and it would be great to get the fix back-ported instead of having to maintain a custom back-port. I support whatever decision the Rails team makes. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment