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

time latency per file #38

Closed
andkaran opened this issue Sep 14, 2013 · 23 comments
Closed

time latency per file #38

andkaran opened this issue Sep 14, 2013 · 23 comments

Comments

@andkaran
Copy link

Average time taken to load per file is around 1 second for me! does anybody else facing this even on fresh install

@evantishuk
Copy link

I would guess this is because it's recompiling your less/css/scss/javascript/coffeescript/ each page load. If you're not actively working with your SCSS/SASS + JS you can turn caching on: https://github.com/CodeSleeve/asset-pipeline/blob/master/src/config/config.php#L110

@andkaran
Copy link
Author

But the same thing also happens if I load precompiled coffee, less,
handlebars files etc. Cache feature solves the problem only when used with
concatenation.

Cache used alone gives more or less the same latency. I am learning and
building a Single page app with lots of scattered js files in different
modules. I can't turn on cache yet.

Can there be any method to first check if original files have changed
before compiling and then send back a 304 not modified header if nothing
changed in a particular file?

This could reduce the load times when in active development.
On Sep 15, 2013 11:10 AM, "evantishuk" notifications@github.com wrote:

I would guess this is because it's recompiling your
less/css/scss/javascript/coffeescript/ each page load. If you're not
actively working with your SCSS/SASS + JS you can turn caching on:
https://github.com/CodeSleeve/asset-pipeline/blob/master/src/config/config.php#L110


Reply to this email directly or view it on GitHubhttps://github.com//issues/38#issuecomment-24464857
.

@evantishuk
Copy link

"Can there be any method to first check if original files have changed before compiling and then send back a 304 not modified header if nothing changed in a particular file?"

I would hope that it worked like that out-of-the-box--given that it's using Assetic which provides a simple built-in caching mechanism (see: https://github.com/kriswallsmith/assetic/blob/master/README.md#internal-caching).

Looking through the code, it seems Asset Pipeline has some concept of caching as evidenced here:

* Else, in order to keep from hammering the file system we use the cache manager

But that doesn't quite cut the mustard and doesn't appear to use Assetic's built in AssetCache.

So, I played around and took a shot at weaving in Assetics Caching approach , at around this part of the SprocketsRepository:

Using the above lines as a reference point, you can edit the relevant logic to look something like:

foreach ($files as $file)
{   
    $base = $this->basePath($file);
    if ($this->isJavascript($base))
    {
        $filters = $this->filters->matching($file);
        $assets[] = new AssetCache(
            new FileAsset($this->getFullPath($base, 'javascripts'), $filters),
            new FileSystemCache(\Config::get('cache.path'));
    }
}

NOTES:

  • You need to add use Assetic\AssetCache; and use Assetic\FilesystemCache; at the beginning of the SprocketsRepository.php file as well.
  • The above snippet edits the getScriptAssets function. To complete the job, you also need to change the corresponding getStyleAssets function as well.
  • Because I did this fast, you may notice that it's hitting your Config file for every request ... a more robust solution would need to avoid that.

So, I thought this would work as-is. But, alas, it exposed what looks like a legitimate issue with Assetic whereby the MinifyCSS filter causes an exception to be thrown due to its having a closure. I looked into that and isolated the cause and started a discussion around a patch. You may wish to join that conversation here: kriswallsmith/assetic#501

So, if you don't want to wait around for the discussion, pull requests, and possible bug fixes before an eventual release (or not)... you can make the changes I suggest above AND the changes I propose over on the Assetic issues list (kriswallsmith/assetic#501), and it should work. I haven't fully tested it, but, it seems to not break and improves loading times.

I am also not the maintainer here and I may be missing something big and obvious and stupid. So, please don't take my suggestions as anything more than the product of 45 minutes worth of poking around from a guy in the peanut gallery.

Cheers.

EDIT: Updated the notes.

@kdocki
Copy link
Contributor

kdocki commented Sep 16, 2013

@evantishuk I actually didn't know about the internal caching from Assetic. I can definitely see the usefulness in caching on per file basis if you had lots of files that needed pre-processing. Also, I don't know how this would mesh with this proposal

@evantishuk
Copy link

@kdocki from what I can tell, I don't think it would impact that proposal too much, if at all. Assetic's AssetCache looks at the pre-compiled file-data and determines if it needs to recompile or pass along the cached content. So, at worst you may have duplicate file-data between your cache folder and public/assets/ which isn't too big of a deal.

That seems like a good default behavior regardless of how many assets you're wrangling. Especially when using something like Bootstrap Sass which has a lot of overhead with the current approach.

Of course, Assetic's AssetCache doesn't handle the MinifyCSS filter at present, so anything in that direction would have to wait for a fix on their end or come up with a workaround that avoids the whole closure issue.

@andkaran
Copy link
Author

Jut tried it. There seems to be some improvement, but I feels it still not enough. From old avg load times of 24s has come down to 19s. Well its still huge for a local dev SPA . Lets hope something better comes out of this

@felixkiss
Copy link
Contributor

@karanits Which filters do you have configured and which are actually used on these time consuming requests?

@andkaran
Copy link
Author

I have configured to use only Coffeescript and handlebars only. I have removed all non necessary paths also

@felixkiss
Copy link
Contributor

What Handlebar filter class do you use?

Can you narrow it down to one of them? Maybe takeout the handlebars filter and try again?

@andkaran
Copy link
Author

actually the filters doesnt matter. I had once pre-complied all coffeescripts and handlebar templates before hand and just used this to load the resulting js files. results being the same latency.

i will try to do that again and see what happens

@andkaran
Copy link
Author

FYI I am using codesleeve's handlebar plug-in. Before that I used to one
provided by Assetic

@andkaran
Copy link
Author

I just tried only loading pre compiled coffee files. Used no filters. Same
type of latency.

I also hard wrote the generated script tags in my blade view and copied all
compiled coffee files to public folder. Instantaneous page reload...
Obviously! I know this was not required but still did it just so.

This just leaves one thing. File system is slowing things down.

@evantishuk
Copy link

Sorry, I'm a little confused now. If you employed the AssetCache as above, it shouldn't recompile files if nothing changed. But, as you've explained, even when its precompiled and devoid of filters it's still slow. It's starting to sound like a peculiarity of your environment. Are you remote mounting to a development server or something?

Can you publish an archive of your assets and structure so we can try to reproduce?

@andkaran
Copy link
Author

Yup. That's what I am thinking. I have a laptop running windows 8 and
Ubuntu server running in it virtually. I do coding in win 8 and push to
the virtual server. So the website is technically severed on a different IP
address

A few days ago, I had wampserver on windows 8, before going virtual. Then
this was all on localhost. Same issue was there to. May be its my laptop
issue?

Well my asset structure is similar to backbonerails.Com tutorials by Brain
Mann. And he makes a lot of folders!

I am loading atleast 20 js files and most of them are less than 100 lines.
Can anyone test just by loading some random js files?

I am sorry, I feel I am not helpful enough.

@evantishuk
Copy link

When I get a chance, I'll try including a few dozen dummy plain JS and coffee files. My local environment is just straight-up Ubuntu, so it should provide a relatively "clean" test. I can also try on my remote dev server which has a lot more horsepower and see how that responds. Even if it turns out that your environment is likely to blame, this discussion (I think) could lead to some real improvements!

@evantishuk
Copy link

@kdocki a fellow over on the Assetic project pointed out that the HashableInterface should be used with any (all?) filters--which may impact how you want to structure the filters in order to reliably leverage Assetic's built in caching and avoid the closure serialization issue. Here's the basic solution using MinifyCSS as an example:

First, adjust the SprocketsRepository to use Assetic's AssetCache (#38 (comment)), from above:

foreach ($files as $file)
{   
    $base = $this->basePath($file);
    if ($this->isJavascript($base))
    {
        $filters = $this->filters->matching($file);
        $assets[] = new AssetCache(
            new FileAsset($this->getFullPath($base, 'javascripts'), $filters),
            new FileSystemCache(\Config::get('cache.path'));
    }
}

Second, the filters should implement HashableInterface. The only way I can think to do this is to extend the current class and implement HashableInterface from there. So, in our case, MinifyCSS becomes MinifyCSSBase and stays unchanged otherwise. Then we create a new MinifyCSS class that looks like:

<?php namespace Codesleeve\AssetPipeline\Filters;

# /asset-pipeline/src/CodeSleeve/AssetPipeline/Filters/MinifyCSS.php

use Assetic\Filter\HashableInterface;

class MinifyCSS extends MinifyCSSBase implements HashableInterface {

  public function hash() { return "MinifyCSS"; }

}

Maybe there's a better way to structure it. I've tested this approach and it works like a charm. If you have a better way to organize and implement the HashableInterface, let me know. Otherwise, I'd be happy to adapt these filters and make a pull request.

@kdocki
Copy link
Contributor

kdocki commented Sep 24, 2013

@evantishuk Thanks for looking into this! :) Did you see an issue too? Also did this HashableInterface will fix the issue if you were able to reproduce it? If so and you want to make those changes ... put in a pull request and I'll definitely push it through.

@kdocki
Copy link
Contributor

kdocki commented Sep 26, 2013

Well... I am going to try to put in some of these changes @evantishuk mentioned and test it... I'm not familar with Assetic Cache stuff but I'd like to see if it helps any.

@karanits right now one thing you can do is set concat => true in the config which I think will help your performance. I have ~200 asset files (I too follow Brian Mann's patterns) and I'm seeing load times of about 9 seconds with 'concat' => null and about 1 second loads for 'concat' => true

@andkaran
Copy link
Author

Concatenation does bring down the load time. But it makes debugging a lot
harder! It brought it down to 10 sec.

Out of frustration I exported the virtual server to another pc and there
load times was 6sec. With concatenation

So some of the issue may be with my laptop being over worked. But still I
expect that load times should not be more than a few seconds even when not
concatenated.

@evantishuk
Copy link

Did you see an issue too?

No, I haven't been able to reproduce the issue as described. :( I can rope in a bunch of JS and CSS and it will indeed chunk about 2 - 4 seconds (annoying, but acceptable IMHO). Nothing in the 20+ second range though.

Also did this HashableInterface will fix the issue if you were able to reproduce it?

I don't know. It definitely improves performance in the average case though.

If so and you want to make those changes ... put in a pull request and I'll definitely push it through.

Karantis stated earlier that the previously suggested monkey-patch I suggested offered a small improvement but not a complete fix. General mood was that something peculiar to his I/O was responsible for the delay. I'm leaning toward his case being rare unless another issue like this gets reported.

I am going to try to put in some of these changes @evantishuk mentioned and test it... I'm not familiar with Assetic Cache stuff but I'd like to see if it helps any.

Let me know how that goes. I think it's the "right" way to do it. I suppose it should probably be the default behavior with a boolean config variable to turn it off in debug mode or explicitly. Also, I'm curious how best to organize the file structure given the HashableInterface implementation. That probably deserves some thought beyond what I suggested.

@andkaran
Copy link
Author

An update:

upon rechecking the edits @evantishuk has mentioned. I noticed my stupid mistake. I forgot to comment out

$assets[] = new FileAsset($this->getFullPath($base, 'javascripts'), $filters); in SprocketsRepository
so effectively I was running both.

Now upon rechecking. I have 60+ assets which are a mix of coffee-scripts and handlebar templates. I get 10sec with no concat and 3sec with concat.

@evantishuk
Copy link

Several things I noticed when testing the Assetic-first caching integration:

First, if you have your CSS manifest looking for, say, pets.scss.css and that file imports child SASS file(s):

@import "_cats.scss";
@import "_dogs.scss";

The cache does not know if _cats.scss or _dogs.scss might have been modified because they're not in the manifest. The work around is to simply update pets.scss.css but the ideal solution would be to scan for SASS imports and check the timestamps on those files as well. Or, perhaps, imports should be avoided and added to the manifest? This is, I think, a minor issue for now.

Second, Assetic allows for a number of possible caching methods https://github.com/kriswallsmith/assetic/tree/master/src/Assetic/Cache (apc, array, file, expiring, etc). This is nice but should these options be exposed through Asset Pipeline?

Third, Assetic could make clearing the cache a challenge. For instance, it's not obvious at all to me if it's feasible or even possible to manually clear the cache that's generated through Assetic. There is a method for removing a cached key (https://github.com/kriswallsmith/assetic/blob/master/src/Assetic/Cache/FilesystemCache.php#L57) but, the function that generates the key is private (https://github.com/kriswallsmith/assetic/blob/master/src/Assetic/Asset/AssetCache.php#L147) and doesn't appear to return the key anywhere outside of that class. Short of mirroring that cache key function within AssetPipeline, I don't see how we could integrate it into the CLI option. I've posted the question (kriswallsmith/assetic#501 (comment)) and hopefully I'm just sleep deprived and missing something obvious.

Fourth, does leveraging the caching methods in Assetic possibly change how caching is done elsewhere in this package? Maybe a bigger question is should we explore making a full use of Assetic's tools and possibly refactor a bunch of existing code? Or maybe stop looking at Assetic and just continue to roll our own?

@kdocki
Copy link
Contributor

kdocki commented Nov 20, 2013

I'm consolidating this issue into #34 so that I don't have multiple issues open. I am going to spend some time experimenting ways to speed up asset pipeline when you have many assets. I'd like to be able to accommodate at least match Rails speed (it can load ~200 assets in 3 second page refresh on my machine).

@kdocki kdocki closed this as completed Nov 20, 2013
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

4 participants