Add cache #47

andyjansson · 2015-06-04T02:18:33Z

Added a cacheDir option. When set, it will enable the cache functionality and store the compiled output(s) along with a dependency graph in the specified directory. Modifying any of the files in the dependency graph invalidates the cache for that file and any dependents. Any unmodified dependencies will continue to be cached.

MoOx · 2015-06-04T05:29:59Z

test/fixtures/cwd.css

-@import "test/fixtures/imports/foo.css";
-@import "test/fixtures/imports/foo-recursive.css";
+@import "imports/foo.css";
+@import "imports/foo-recursive.css";


this test should not be changed since the point of this one is to check that without from option, you can use a file from your cwd (which is the root of this project when running test from the root)

Then I propose that the documentation should be altered to reflect that:

// `from` option is required so relative import can work from input dirname

it say "input", not "cwd" which is correct. If you make from a file import "./stuff" and provide the from option, the file might be found in the same dir as the original input, and this cannot be done if you don't provide the from option.
What is not correctly clear ?
There is a test for that

postcss-import/test/index.js

Line 62 in debda62

compareFixtures(t, "relative-to-source", "should not need `path` option if `source` option has been passed to postcss", null, {from: "test/fixtures/relative-to-source.css"})

andyjansson · 2015-06-05T15:59:05Z

ping @MoOx

MoOx · 2015-06-05T19:29:17Z

test/index.js

+  expected = read("fixtures/" + name + ".expected")
+  t.equal(actual, expected, "read content from cache")
+  t.end()
+})


how is this testing that the cache has been used ?!

The first process() call puts the compiled output into the cache since cacheDir has been specified.

The second process() call rather than compiling the CSS again instead reads it from the cache.

The test does not prove anything right now :/. We should probably add something to track the cache usage.

andyjansson · 2015-06-08T18:12:07Z

@MoOx The test has been updated to include a check of the cache's state.

MoOx · 2015-06-08T22:05:00Z

test/index.js

+  var cacheFileName = imports[cssFileName].cache;
+  var cache = fs.readFileSync(cacheFileName, "utf8").trim()
+
+  t.equal(output, cache, "should put output in cache")


we know here that the cache is written, how do we know the cache is being read after that ?

I'm sorry @MoOx, there's no way of knowing. It's a transparent cache. Unless you're suggesting that we should add a bunch of debug code to poll the internal state of the engine, which I would strongly advice against, we might as well close the PR because there's no way for me to satisfy this need. I know that the cache is hit by using breakpoints, I know that it returns a correct and consistent result, and I know that I get a 100ms performance increase when using it with a local project of mine, but there's no way for me to put that into a unit test.

There is always a way. What about adding a simple map object with {file-requested: bool flag} to know if a requested file as been used from cache? Doesn't seems hard to do and this won't affect perfs.
If you don't know how to do it, I can probably handle that.

I don't quite understand what you mean, but you're welcome to take a stab at it.

Fix plugins section of README

MoOx · 2015-06-21T05:58:41Z

I was about to merge that to handle the missing test but we made some changes in the master. Did you want to handle the rebase? Otherwise I will try to do it on tuesday.

Fix warnings

jednano · 2015-06-24T05:14:00Z

@andyjansson you gonna' rebase this one?

andyjansson · 2015-06-24T13:08:43Z

@jedmao I tried. I give up. I'll leave this to @MoOx

andyjansson · 2015-06-27T19:31:54Z

@MoOx @jedmao I have a working version based on the latest source, however I haven't rebased the project. I'm thinking it might be easier to just create a new fork and add the code there. What do you think?

jednano · 2015-06-27T19:46:56Z

@andyjansson did you see #62? I basically handled the rebasing there. You can cherry-pick my commits.

andyjansson · 2015-06-27T20:06:52Z

@jedmao you should probably PR to my repo

andyjansson · 2015-06-27T23:23:53Z

Don't merge this yet. I need to verify that the cache works as intended.

andyjansson · 2015-06-28T00:23:25Z

@jedmao Enabling caching for all tests and running the tests twice causes should be able to consume npm package or local modules to fail. It looks like it isn't ignoring an @import declaration for an already imported document. Having another set of eyes on this issue would be swell.

jednano · 2015-06-28T00:52:36Z

.gitignore

 test/fixtures/*.actual.css
+test/cache


We have a duplicated line here that needs to be removed.

jednano · 2015-06-28T00:53:18Z

Are you familiar w/ how to rebase? The merge commits in this PR are, I think, something we want to avoid.

jednano · 2015-06-28T01:05:47Z

See #63 for a clean rebase w/o merge commits. Feel free to submit a PR to my cache branch with the failing test you're talking about. I'm not sure what the issue is there.

andyjansson · 2015-06-28T01:46:09Z

@jedmao It's not ready to be merged, so there would be no point in squashing the commits before it is. Once the issue mentioned earlier has been resolved I'll do an interactive rebase.

jednano · 2015-06-28T02:08:16Z

I'd like to see a failing build w/ the issue you're speaking of.

andyjansson · 2015-06-28T22:12:56Z

@jedmao Just change line 19 of test/index.js to the following and run npm test twice:

  opts = assign({path: importsDir, cacheDir: cacheDir}, opts || {})

jednano · 2015-07-01T01:00:36Z

@andyjansson I see what you mean now. Unfortunately, I don't know enough about how the caching works to really offer any help. My involvement in this PR was in response to #59 (comment), to get the ball rolling with my index.js refactor. FWIW, I'm getting the same failing test as you on the 2nd npm test run.

…cache

andyjansson · 2015-07-27T21:56:35Z

@jedmao Well, I finally had time to fix the bug, so the good news is that everything is working. The bad news is that the fork is now behind the upstream repo, which is why appveyor is reporting an error.

poeschko · 2017-05-03T10:06:04Z

May I ask why this was closed? Does it not work correctly, would it need more work on the PR (rebasing it again), or do you think it would not provide any significant advantage over the improvements that have already been made? It seems to me that #152 only achieves parts of what a more general caching approach would (only caching file I/O, but not any transformations applied after that).

Using postcss-devtools, I found that postcss-import takes ~20 seconds in my project (accumulated over multiple CSS entry points), which is the vast majority of the overall build time. So any speedup would be great, and I'd be motivated to help if desired.

RyanZim · 2017-05-03T12:37:35Z

@poeschko This PR was closed because of merge conflicts. Since then, the entire codebase was rewritten. No one ever attempted to replicate this feature.

I'd welcome any performance improvements.

There's two big performance problems in postcss-import:

Poor Caching, only caches file I/O, not processing
This line: https://github.com/postcss/postcss-import/blob/master/index.js#L177. That promiseEach serializes a lot of stuff that could run concurrently (in theory). It was added as a quick fix for a bug in the global deduping, but there's probably a more performant way to do it.

I'm not sure which problem is the biggest slowdown.

andyjansson · 2017-05-03T14:54:02Z

@poeschko I closed the PR because I felt the feature was never going to get merged in, so I switched my focus to other things.

tqwewe · 2019-02-23T08:23:27Z

This feature would be amazing. Especially in my case because I am importing src from a node module which adds a huge delay to my app.

AndyOGo · 2020-12-11T10:36:39Z

This would be a great addition.

andyjansson added 2 commits June 4, 2015 03:43

Fix tests

fe1a753

Add cache

f183e59

andyjansson mentioned this pull request Jun 4, 2015

Async FS calls to improve perfomance #41

Closed

MoOx reviewed Jun 4, 2015
View reviewed changes

MoOx mentioned this pull request Jun 4, 2015

Add caching to improve performance #48

Closed

Move cache to its own test

190df56

MoOx reviewed Jun 5, 2015
View reviewed changes

Add test to check cache contents

c8c3bbd

MoOx reviewed Jun 8, 2015
View reviewed changes

jednano and others added 2 commits June 19, 2015 17:22

Fix plugins section of README

b69f080

Merge pull request #58 from jedmao/fix-readme

177ef2e

Fix plugins section of README

MoOx mentioned this pull request Jun 21, 2015

Refactor index.js #59

Closed

Merge pull request #57 from jedmao/fix-warnings

466ce1f

Fix warnings

jednano added 2 commits June 24, 2015 15:01

Refactor index.js

3642fff

Introduce caching

389121c

jednano mentioned this pull request Jun 24, 2015

Introduce caching #62

Closed

andyjansson added 3 commits June 28, 2015 01:12

merge jedmao/postcss-import

b1a48c5

Merge branch 'jedmao-cache'

4bc907b

Fix cache

936a93e

Fix cache

d0b054c

jednano reviewed Jun 28, 2015
View reviewed changes

.gitignore

test/fixtures/*.actual.css

test/cache

Copy link

Contributor

jednano Jun 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a duplicated line here that needs to be removed.

jednano mentioned this pull request Jun 28, 2015

Introduce Caching #63

Closed

andyjansson added 2 commits July 27, 2015 20:52

Fix for not invoking files with same content twice when reading from …

20bced0

…cache

Fix for putting file read from cache into hashFiles

00cac1b

zoubin mentioned this pull request Aug 18, 2015

add options.cache to save file io #74

Closed

mstade mentioned this pull request Nov 3, 2015

Stop cloning options for a massive performance boost #90

Merged

MoOx mentioned this pull request Jan 11, 2016

Add read-cache to load files #152

Merged

andyjansson closed this Jan 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cache #47

Add cache #47

andyjansson commented Jun 4, 2015

MoOx Jun 4, 2015

andyjansson Jun 4, 2015

MoOx Jun 4, 2015

andyjansson commented Jun 5, 2015

MoOx Jun 5, 2015

andyjansson Jun 6, 2015

MoOx Jun 6, 2015

andyjansson commented Jun 8, 2015

MoOx Jun 8, 2015

andyjansson Jun 9, 2015

MoOx Jun 9, 2015

andyjansson Jun 9, 2015

MoOx commented Jun 21, 2015

jednano commented Jun 24, 2015

andyjansson commented Jun 24, 2015

andyjansson commented Jun 27, 2015

jednano commented Jun 27, 2015

andyjansson commented Jun 27, 2015

andyjansson commented Jun 27, 2015

andyjansson commented Jun 28, 2015

jednano Jun 28, 2015

jednano commented Jun 28, 2015

jednano commented Jun 28, 2015

andyjansson commented Jun 28, 2015

jednano commented Jun 28, 2015

andyjansson commented Jun 28, 2015

jednano commented Jul 1, 2015

andyjansson commented Jul 27, 2015

poeschko commented May 3, 2017

RyanZim commented May 3, 2017

andyjansson commented May 3, 2017

tqwewe commented Feb 23, 2019

AndyOGo commented Dec 11, 2020

Add cache #47

Add cache #47

Conversation

andyjansson commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyjansson commented Jun 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyjansson commented Jun 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoOx commented Jun 21, 2015

jednano commented Jun 24, 2015

andyjansson commented Jun 24, 2015

andyjansson commented Jun 27, 2015

jednano commented Jun 27, 2015

andyjansson commented Jun 27, 2015

andyjansson commented Jun 27, 2015

andyjansson commented Jun 28, 2015

Choose a reason for hiding this comment

jednano commented Jun 28, 2015

jednano commented Jun 28, 2015

andyjansson commented Jun 28, 2015

jednano commented Jun 28, 2015

andyjansson commented Jun 28, 2015

jednano commented Jul 1, 2015

andyjansson commented Jul 27, 2015

poeschko commented May 3, 2017

RyanZim commented May 3, 2017

andyjansson commented May 3, 2017

tqwewe commented Feb 23, 2019

AndyOGo commented Dec 11, 2020