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

Performance for large projects -> improved ten times! #504

Closed
janbiedermann opened this issue Oct 12, 2017 · 8 comments
Closed

Performance for large projects -> improved ten times! #504

janbiedermann opened this issue Oct 12, 2017 · 8 comments

Comments

@janbiedermann
Copy link

janbiedermann commented Oct 12, 2017

ruby-hyperloop.org project uses rails with opal-rails, opal being ruby for browsers then makes the complete ruby load_path the asset path for sprockets. In rails development environment then sprockets checks with File.fstat the whole asset path/ruby load_path, thousands of files on each request or javascript_include_tag resulting in devastating response times.

sprockets should have a configurable option, to specify the directory or directories on which it should check for changed files and cache the File.stat for files outside of those directories. Example monkey provided below.

Numbers: for simple page get with sprockets 3.7.1 around 4 seconds
With monkey patch provided below around 480ms
(including #503 )

monkey patch:
sprockets-3.7.1/lib/sprockets/path_utils.rb

  module PathUtils
    extend self

    def stat(path)
      root = '/put/rails/root/path/here/app'
      if (path[0..(root.size-1)] != root)
        @@gorilla_patch_cached_stats ||= {}
        return @@gorilla_patch_cached_stats[path] if @@gorilla_patch_cached_stats.has_key?(path)
        @@gorilla_patch_cached_stats[path] = if File.exist?(path)
                                               File.stat(path)
                                             else
                                               nil
                                             end
      else
        if File.exist?(path)
          File.stat(path)
        else
          nil
        end
      end
    end
@janbiedermann
Copy link
Author

janbiedermann commented Oct 12, 2017

Devastating because, people coming with their existing rails apps to ruby-hyperloop, just including the hyperloop gem, experience page load times of 40 seconds, just because they have several asset_tags in their app.

@ahorek
Copy link
Contributor

ahorek commented Oct 12, 2017

hmm, are you sure this is the right place?

I can't simulate it, this method is called only on compiled files in 'public/assets/' directory during assets:precompile and only once for each file, so it should be fine
Development (without cache) or production mode doesn't use this method at all

@janbiedermann
Copy link
Author

janbiedermann commented Oct 12, 2017

I am sure its not the right place, the monkey above is just to express what i mean and to allow others to verify my finding with opal-rails or ruby-hyperloop.

Please, in your rails app, do a gem 'hyperloop' and measure in rails dev.

Its not javascript assets from public/assets, its full fledged ruby in the browser during development for serious front end development in rails development environment, where page load times in double digits are painful

@ahorek
Copy link
Contributor

ahorek commented Oct 13, 2017

@janbiedermann thanks, but instead of caching, calling these stats should be avoided

the problem is that this peace of code calls file stats (for the whole structure) if path doesn't contain '/'
https://github.com/rails/sprockets/blob/master/lib/sprockets/bower.rb#L24
but in most cases it fails (we don't use it at all)

here's the second case
https://github.com/rails/sprockets/blob/3.x/lib/sprockets/resolve.rb#L155
is calls stats many many times for each possible combination. This is the biggest performance killer, but only on development. On production these paths are either cached or static.

It looks like sprockets 4 has a better resolver, but I didn't have a chance to test it because of compatibility issues.

@janbiedermann
Copy link
Author

@ahorek that would be just perfect, if those calls could be avoided, i just don't understand sprockets that well to find to right place, so thanks for pointing me there. I don't know what bower is, but i will tinker with resolve.rb#L155 and sprockets 4.

@janbiedermann
Copy link
Author

if i have to cache the cache to get at least a little bit performance, then something is wrong with the cache, isn't it?
i believe it is possible to get decent compile times with opal, meaning below 1 second, though i see no way with current sprockets 3 or 4 design.
numbers in #507

@janbiedermann
Copy link
Author

janbiedermann commented Oct 15, 2017

design issues:

  • the limitation of the internal MemStore to a hardcoded 1024 size, which makes, with constant trashing as in my use case with ruby-hyperloop, even the FileCache useless.
  • the attachment of above MemStore to every other Store
  • accessing the filesystem for every lookup, even many times per lookup

Alternatives provided in PR #525

@janbiedermann janbiedermann mentioned this issue Nov 25, 2017
@janbiedermann
Copy link
Author

i got a opal-webpack-loader instead

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

No branches or pull requests

2 participants