Cache saving error on mixins with arguments still present under 3.1.0 about sass-rails HOT 36 CLOSED

We'll get this sorted out.

from sass-rails.

Thanks Chris, let me know if you need any more details at any point.

from sass-rails.

Can you zip up the smallest rails app that exemplifies this and email it to me? [email protected]

from sass-rails.

Sure, will do that later today.

from sass-rails.

@chriseppstein Did you see the latest 3 comments by @markiz in #5. Looks like he has narrowed down the problem.

from sass-rails.

@spohlenz we have recently made a change to sass that is similar. @olivierlacan I assumed you have sass 3.1.7 installed. Confirm/Deny?

from sass-rails.

Confirm. Running Sass 3.1.7.

On Wednesday, August 31, 2011 at 7:18 PM, chriseppstein wrote:

@spohlenz we have recently made a change to sass that is similar. @olivierlacan I assumed you have sass 3.1.7 installed. Confirm/Deny?

Reply to this email directly or view it on GitHub:
#36 (comment)

from sass-rails.

I created a new 3.1 app the other day to try to reproduce the issue, and couldn't.

I didn't try to make a 3.0.x app and upgrade to Rails 3.1 which seems in hindsight to be a better path to reproduction.
I have two apps showing this same issue:

• one was under 3.0.x and upgraded to 3.1 recently
• one was created on an early 3.1 RC (or Beta) and upgraded slowly to 3.1 stable.

I'm still in the dark as to what could be causing this, and I tried @markiz's fix to no avail.

from sass-rails.

My investigation on this issue :

• A Sass::CacheStores::Base instance attempts (and fails) to call Marshal.dump on :
• A Sass::Tree::RootNode instance which amongst its @children nodes has :
• A Sass::Tree::MixinDefNode instance which amongst its @args has :
• A Sass::Script::Variable instance which amongst its @options has :
• A Sass::Rails::Resolver instance in the :custom @options key as well as a Sass::Rails::Importer in the :load_paths @options key (which ends up instantiating a Sass::Rails::Resolver of its own), both of which :
• Are initialized with a @context variable from Tilt and which happens to be an instance of Sprockets::Context which in turn was initialized with :
• An @environment which is a Sprockets::Environment instance variable which contains :
• Hashes initialized with a default proc in its @*processors variables, which cannot be marshalled.

I'm not familiar enough with the internals of all these libraries to know where to fix this, but I'd be happy to help test or debug.

from sass-rails.

Thanks a lot @srgpqt for the additional info.

I spent the entire week end reinstalling my dev environment on a new machine. I used rbenv instead of RVM. As evident from the lack of "+1s" here, the issue is very dependent on the environment.

I have absolutely no issue (so far) under this new environment (running 1.9.2-p290 and 3.1). So I'm out of the woods, but this is still a troubling issue.

from sass-rails.

@nex3 I think we must have missed a reference to the options hash somewhere :'(

from sass-rails.

It looks like the options hash isn't getting set to {} before the file is getting marshaled. However, I'm not sure why this would be happening; Sass::Engine always sets it to {}, and I can't see that anything would be calling the cache other than Sass::Engine.

@srgpqt Can you provide a backtrace for the error?

from sass-rails.

Here's that backtrace. I'll be doing some more debugging and get back to you with what I've found.

https://gist.github.com/1249757

from sass-rails.

Upon doing some more debugging, I found that there was a third Sass::Rails::Importer instance under the :importer key of @options hash.

I have confirmed that Marshal.dump successfully dumps the root node when those three keys are removed from the @options hash, or if the options hash is removed altogether.

from sass-rails.

@srgpqt That backtrace implies that there's something wrong with Sass::Tree::Visitors::SetOptions, which should be setting @options to null everywhere before caching.

Can someone send me a Rails app that demonstrates this issue? I'm still unable to reproduce it locally. Either post it in a gist or repo, or just email it to me at [email protected].

from sass-rails.

Here you go.
Be sure to delete the contents of your rails tmp directory before trying to reproduce the bug.

https://github.com/srgpqt/GrumpyCache

from sass-rails.

sass/sass@5fc56f3 fixes this. As I suspected, Visitors::SetOptions was broken.

from sass-rails.

You guys get dinner on me if you're ever in Orlando. Thanks so much :-)

from sass-rails.

@nex3 fix confirmed, thank you.

from sass-rails.

I don't mean to alarm anyone. But after successfully upgrading Sass to 3.1.8 I started noticing huge slow downs in compile time once again.

And that's after playing with live-reload and having styles refresh almost instantly for a while on Friday.

Rendered service_options/index.html.haml within layouts/application (1559.5ms)
Compiled application.css  (14ms)  (pid 36161)
Compiled assets.css  (63ms)  (pid 36161)
Compiled devices.css  (3075ms)  (pid 36161)
Compiled galleries.css  (1ms)  (pid 36161)
Compiled hotels.css  (4651ms)  (pid 36161)
Compiled info.css  (2649ms)  (pid 36161)
Compiled info_pages.css  (1ms)  (pid 36161)
Compiled info_sections.css  (1ms)  (pid 36161)
Compiled normalize.css  (19ms)  (pid 36161)
Compiled orders.css  (2151ms)  (pid 36161)
Compiled pages.css  (229ms)  (pid 36161)
Compiled scaffolds.css  (65ms)  (pid 36161)
Compiled service_options.css  (2322ms)  (pid 36161)
Compiled services.css  (2196ms)  (pid 36161)
Compiled style.css  (16005ms)  (pid 36161)
Compiled users.css  (2416ms)  (pid 36161)
Compiled jquery-ui-1.8.7.custom.css  (1ms)  (pid 36161)
Compiled jquery.qtip.css  (1ms)  (pid 36161)
Compiled uploadify.css  (0ms)  (pid 36161)
Compiled application.js  (35ms)  (pid 36161)
(...)
Completed 200 OK in 39788ms (Views: 39524.5ms | ActiveRecord: 11.7ms)

Tried that on both 1.9.3-rc1 and 1.9.2-p290.
And sadly, the only common denominator between those stylesheets is the presence of a @import compass on top of them.

Removing the compass mixins (and import) I get this:

Compiled hotels.css  (2481ms)  (pid 36161)

If I just remove my mixin stylesheet import, I get:

Compiled hotels.css  (2394ms)  (pid 36161)

Not much difference there.
But if i remove an import for my own mixin (with args) stylesheet AND compass I get:

Compiled hotels.css  (158ms)  (pid 36161)

Can anybody reproduce this?

from sass-rails.

sass-rails searches sprockets first for imports because it's a local resolution, then it searches filesystem paths like compass. My guess is that sprockets is slow at resolving imports. But we should profile it.

from sass-rails.

@olivierlacan I too have noticed a similar slowdown recently. Unfortunately I don't have any more info at this stage.

from sass-rails.

I'm also seeing a huge slowdown. I've heard downgrading to sass 3.1.0 solves some of the slowdown (http://www.sencha.com/forum/showthread.php?139957-Compass-Sass-compiling-unbearably-slow), but I can't downgrade because sass-rails requires 3.1.4 and above.

from sass-rails.

@whather Can you try compiling your stylesheets without using the Rails infrastructure to see if that's similarly slow? Also, how slow are we talking?

from sass-rails.

@nex3 about 10 seconds average compiling stylesheets within the Rails 3.1 infrastructure. It's much faster without using the Rails infrastructure

from sass-rails.

In that case, Chris is probably right that Sprockets is at fault.

from sass-rails.

This is on my list to investigate in the near future. I think it is
sprocket's fault, but I have to prove it :(

from sass-rails.

This issue is back in 3.3.0 -- Sass::Tree::ImportNode contains a reference to Sass::Rails::Importer which brings us back to the original issue described by @srgpqt.

from sass-rails.

The following seems to fix it. Any harm in it?

module Sass
  module Tree
    class ImportNode < RootNode
      def _dump(f)
        Marshal.dump([@imported_filename, children])
      end

      def self._load(data)
        filename, children = Marshal.load(data)
        node = ImportNode.new(filename)
        node.children = children
        node
      end
    end
  end
end

from sass-rails.

@cheald : I think this fix should go to the sass repository instead.

from sass-rails.

Ah, yes, it should. I had way too many repos open when tracking this one down -- sorry. :)

from sass-rails.

No problem, thanks for reporting and trying to enhance these projects!

from sass-rails.

@robin850, @cheald I think this could actually be a bug in Sprockets.

According to https://github.com/nex3/sass/blob/master/lib/sass/importers/base.rb#L17, Sass importers should be serializable via Marshal.dump. However Sprockets::SassImporter (https://github.com/sstephenson/sprockets/blob/master/lib/sprockets/sass_importer.rb) contains the context class as an instance variable which can't be marshalled (and the Sass Importer inheritance chain does not define custom _dump/_load methods).

I think the correct fix would be to define _dump and _load on Sprockets::SassImporter in the sprockets repo.

This isn't to say that @cheald's patch (sass/sass#1029) isn't also necessary.

from sass-rails.

There's a second issue in that there's an eventual reference to Trail, which contains a Hash with a default proc, which is also not serializable. That may be fixable by the proposed _dump/::load on Sprockets::SassImporter, though -- I don't have the reference hierarchy in front of me to say conclusively at the moment.

from sass-rails.

So far in my experimentation, defining some dumb (i.e. ignoring the Sprockets context) marshalling methods on Sprockets::SassImporter, seems to work but very likely has some side affects I'm not aware of.

class Sprockets::SassImporter
  def _dump(level); @root; end
  def self._load(args); new(nil, args); end
end

On a project with a large number of @imports, this prevents the warnings and reduces compilation time by roughly half.

Btw, this is on rails 4.0.1, sprockets 2.10.1, sass-rails 4.0.1, sass 3.3.0.rc.2.

There's a second issue in that there's an eventual reference to Trail, which contains a Hash with a default proc, which is also not serializable.

Yes, I'm seeing this too (can't dump hash with default proc) when defining a mixin with variable arguments. e.g.

=this_mixin_fails($args...)
  color: red

This issue appears different to the first issue though, and isn't fixed by adding the above methods on Sprockets::SassImporter.

from sass-rails.

Regarding Sprockets' load time, I picked up an additional 20-25% performance by patching Ruby's Pathname class - it's offensively inefficient, and Sprockets leans way heavily on it. With that and the Sass marshalling patch in place, my render times are significantly faster. I'm doing my rendering through Guard, so that the actual page refreshes are faster, and it's made a pretty significant difference in the speed of my workflow.

(I recognize this isn't relevant to this bug in particular, but given the speed gain mention above, I figured it was appropriate)

sstephenson/sprockets#506

from sass-rails.