This repository has been archived by the owner. It is now read-only.

Pathname usage causes significant performance degredation #506

Closed
cheald opened this Issue Nov 28, 2013 · 9 comments

Comments

Projects
None yet
3 participants
@cheald

cheald commented Nov 28, 2013

Ruby's Pathname module is notoriously slow, and Sprockets makes pretty heavy use of it, which imposes a pretty hefty performance penalty when doing asset resolution and compilation.

I've been tinkering with a Pathname monkeypatch in an attempt to improve Sprockets' performance. I realize that this is more of a stdlib concern than it is a Sprockets concern, but Sprockets could realize some immediate performance gains by reducing or eliminating Pathname usage.

I'm testing this via Guard with body = Rails.application.assets.find_asset(file).body - nothing all that complex.

I'm comparing this against a more direct Sass invocation, via:

context = ::Rails.application.assets.context_class.new(::Rails.application.assets.context_class, file, file)
options = @config.merge(custom: { resolver: ::Sass::Rails::Resolver.new(context) })
body = ::Sass::Engine.for_file(file, options).render

Pathname usage on the Sprockets codepath (unpatched):

image

Pathname usage on the Sprockets codepath (patched):

image

And Pathname usage on the more direct codepath:

image

The patch I'm working with is pretty straightforward - I'm just patching #join to not do the whole set of Pathname gymnatics. It still passes the Ruby pathname test suite, too! (Though it does take a shortcut by dropping Windows support; that's easily fixed by checking for File::ALT_SEPARATOR as well)

require 'pathname'
class Pathname
  def relative?
    @path[0] != File::SEPARATOR
  end

  def join(*args)
    last = args.last
    if last.to_s[0] == File::SEPARATOR
      if last.is_a? Pathname
        last
      else
        Pathname.new last
      end
    else
      Pathname.new(File.join @path, *args.map(&:to_s))
    end
  end
end

And some quick numbers:

# Standard sprockets
20:07:12 - INFO - [1/1] (2.09 sec)      wufoo/default.css.sass -> wufoo/default.css
20:07:16 - INFO - [1/1] (2.04 sec)      wufoo/default.css.sass -> wufoo/default.css
20:07:20 - INFO - [1/1] (2.06 sec)      wufoo/default.css.sass -> wufoo/default.css
20:15:13 - INFO - [5/5] (6.55 sec)                     app.css.sass -> app.css
20:15:38 - INFO - [5/5] (5.64 sec)                     app.css.sass -> app.css
20:15:57 - INFO - [5/5] (6.00 sec)                     app.css.sass -> app.css

# With Pathname patches
20:06:26 - INFO - [1/1] (1.53 sec)      wufoo/default.css.sass -> wufoo/default.css
20:06:30 - INFO - [1/1] (1.50 sec)      wufoo/default.css.sass -> wufoo/default.css
20:06:33 - INFO - [1/1] (1.53 sec)      wufoo/default.css.sass -> wufoo/default.css
20:17:25 - INFO - [5/5] (5.03 sec)                     app.css.sass -> app.css
20:17:37 - INFO - [5/5] (5.03 sec)                     app.css.sass -> app.css
20:17:58 - INFO - [5/5] (5.12 sec)                     app.css.sass -> app.css

It should be pretty obvious that leaning on Pathname so hard is causing some pretty measurable performance degradation. It may be worth moving to File methods or even just custom high-performance helpers if at all possible.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Jan 29, 2014

Contributor

Though it does take a shortcut by dropping Windows support

Thats basically why I'd like to use Pathname. I'd really like to avoid baking in windows specific conditions into sprockets since I'd rather now spend any extra time debugging those issues.

Unless you want to volunteer to look into any windows issue that are opened on sprockets and help maintain that code path?

Contributor

josh commented Jan 29, 2014

Though it does take a shortcut by dropping Windows support

Thats basically why I'd like to use Pathname. I'd really like to avoid baking in windows specific conditions into sprockets since I'd rather now spend any extra time debugging those issues.

Unless you want to volunteer to look into any windows issue that are opened on sprockets and help maintain that code path?

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jan 29, 2014

It would be just a couple of extra lines in my patch to properly support Windows; I just didn't since I'm developing and deploying on Linux.

I think I could build a Pathname facsimile (perhaps a separate library?) that would be significantly faster; it would only need to implement the common functionality that Sprockets uses, and would work properly across platforms. Would there be interest in that?

cheald commented Jan 29, 2014

It would be just a couple of extra lines in my patch to properly support Windows; I just didn't since I'm developing and deploying on Linux.

I think I could build a Pathname facsimile (perhaps a separate library?) that would be significantly faster; it would only need to implement the common functionality that Sprockets uses, and would work properly across platforms. Would there be interest in that?

@timmfin

This comment has been minimized.

Show comment
Hide comment
@timmfin

timmfin Feb 2, 2014

Googling around about pathname performance in ruby brought me to pathname3. It seems old, and I'm not sure if it has windows support either, but it might be of use.

timmfin commented Feb 2, 2014

Googling around about pathname performance in ruby brought me to pathname3. It seems old, and I'm not sure if it has windows support either, but it might be of use.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Feb 9, 2014

Contributor

I think going forward I'm going to write less code around Pathname, but I can't commit to a major refactor. Hopefully the situation improves as code is cleaned up.

Contributor

josh commented Feb 9, 2014

I think going forward I'm going to write less code around Pathname, but I can't commit to a major refactor. Hopefully the situation improves as code is cleaned up.

@josh josh closed this Feb 9, 2014

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Feb 10, 2014

Since this doesn't look like it'll be fixed soon, I've published faster_pathname as an interim solution to hack around these issues.

cheald commented Feb 10, 2014

Since this doesn't look like it'll be fixed soon, I've published faster_pathname as an interim solution to hack around these issues.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Apr 19, 2014

Contributor

Making some progress on Pathname reduction. Pathname#absolute? checks are still killing me.

def absolute?(path)
  path[0] == File::SEPARATOR
end

I wish, but I don't think this covers it. This probably needs to match C:/stuff on Windows.

Anyone have more ideas?

Contributor

josh commented Apr 19, 2014

Making some progress on Pathname reduction. Pathname#absolute? checks are still killing me.

def absolute?(path)
  path[0] == File::SEPARATOR
end

I wish, but I don't think this covers it. This probably needs to match C:/stuff on Windows.

Anyone have more ideas?

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Apr 19, 2014

I'm actually running on Windows at the moment, so I can run the test suite and see if it passes. I'll wire that up real fast.

cheald commented Apr 19, 2014

I'm actually running on Windows at the moment, so I can run the test suite and see if it passes. I'll wire that up real fast.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Apr 19, 2014

Contributor

Hike seems to be the bigger offender.

sstephenson/hike#32

Compiling github's assets produces like 20,000 Pathname#join calls with the majority in Hike. #565 cleans up a few pieces of low hanging fruit in Sprockets.

Contributor

josh commented Apr 19, 2014

Hike seems to be the bigger offender.

sstephenson/hike#32

Compiling github's assets produces like 20,000 Pathname#join calls with the majority in Hike. #565 cleans up a few pieces of low hanging fruit in Sprockets.

@josh josh closed this in #565 Apr 19, 2014

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Apr 20, 2014

Make sure you nuke your tmp/cache between tests. When the cache is stale (ie, during active development), there are some sticky spots that don't involve hike. This is particularly relevant in light of #507 / 655f129 which will mean that Sass will not be able to leverage the cache properly.

In Sprockets in particular, the two outstanding non-hike spots are:

https://github.com/sstephenson/sprockets/blob/v2.2.1/lib/sprockets/asset_attributes.rb#L17-L27
https://github.com/sstephenson/sprockets/blob/v2.2.1/lib/sprockets/context.rb#L83

Sass's file importer spams the everliving crap out of Pathname methods, as well, so I need to open a PR there. They are bypassed with a fresh cache, though.

cheald commented Apr 20, 2014

Make sure you nuke your tmp/cache between tests. When the cache is stale (ie, during active development), there are some sticky spots that don't involve hike. This is particularly relevant in light of #507 / 655f129 which will mean that Sass will not be able to leverage the cache properly.

In Sprockets in particular, the two outstanding non-hike spots are:

https://github.com/sstephenson/sprockets/blob/v2.2.1/lib/sprockets/asset_attributes.rb#L17-L27
https://github.com/sstephenson/sprockets/blob/v2.2.1/lib/sprockets/context.rb#L83

Sass's file importer spams the everliving crap out of Pathname methods, as well, so I need to open a PR there. They are bypassed with a fresh cache, though.

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