Possible memory leaks #1669
Comments
|
@danez would that be possible to drop some code to demonstrate the problem? libsass has various issues with regards to the memory management and it would be great to track this one down. |
|
Thanks for the response. I'm going to talk to our Head of Engineering next week and see how we can proceed here. Do you have an email address where we would be able to reach out to you? You can also shoot me an email, mine is in my profile. |
|
Sure, you can reach me at |
|
Okay I'm going to summarize what we found out while investigating:
(The memory is in % to easily see the difference 30% is around 1.3GB) Here is a test sample that reproduces the big map problem: @import './random-map';
@for $i from 1 through 500 {
@debug map-get($map, "0f99ae3b-147f-4bc4-93aa-4e62b31b380c");
@debug $i;
}The big map can be found here: https://gist.github.com/danez/a979b74702e3057c04a9086a994e5962 And attached is the output of valgrind —tool=massif thanks also to my college @detonator413 who did all the investigation. |
|
This should be addressed by sass/libsass#2171 |
|
Thank you. The example I posted above now does free up the memory after finish, but the peak memory usage is still around 1.3GB for this simple example. Our initial problem stays exactly the same and still does OOM errors and does not free the memory. So we are going to do more investigation on this why. |
|
Okay here is another stripped down example: @import './random-map';
@each $entry in $map {
$value : map-get($map, $entry);
}This is even worse, as with more than 3k items in the map it always runs into The Should I create a new Issue for this? In libsass/node-sass? Or is it okay here? |
|
As @mgreter mentioned we have made improved to memory in sass/libsass#2171. You can compile node-sass with the latest LibSass using the following diff. diff --git a/package.json b/package.json
index df179e1..bb97391 100644
--- a/package.json
+++ b/package.json
@@ -1,7 +1,7 @@
{
"name": "node-sass",
- "version": "3.10.0",
- "libsass": "3.3.6",
+ "version": "3.11.0",
+ "libsass": "3.4.0",
"description": "Wrapper around libsass",
"license": "MIT",
"bugs": "https://github.com/sass/node-sass/issues",
diff --git a/src/libsass b/src/libsass
index 3ae9a20..2fcd639 160000
--- a/src/libsass
+++ b/src/libsass
@@ -1 +1 @@
-Subproject commit 3ae9a2066152f9438aebaaacd12f39deaceaebc2
+Subproject commit 2fcd6390a9d94949695ee99aa823781d53ea1760
diff --git a/src/libsass.gyp b/src/libsass.gyp
index fe22917..ce5729e 100644
--- a/src/libsass.gyp
+++ b/src/libsass.gyp
@@ -15,6 +15,7 @@
'libsass/src/base64vlq.cpp',
'libsass/src/bind.cpp',
'libsass/src/cencode.c',
+ 'libsass/src/check_nesting.cpp',
'libsass/src/color_maps.cpp',
'libsass/src/constants.cpp',
'libsass/src/context.cpp',You build new binary with |
|
All my tests today were done with a selfcompiled node-sass from master and I thought this includes the latest libsass. Will try with your diff. |
|
Okay I finally compiled the correct version, but now our codebase immediately throws I tried increasing the max-stacklevel to 1000 but same problem. |
|
This error appears in a different file with different locations on each run with If I change |
|
Just to pitch in, my company is seeing a very similar issue. Our theming system is heavily based on sass maps. We can see upwards of 6 gigs of usage when building any given brand css file out. As a little insight, we store pretty much all of our styles inside sass maps which are then pulled in via a system of mixins. We're actively looking into solutions or migration strategies to combat this. |
|
@danez since 4 has been released, it might be worth testing again because I believe there where a few performance related bits that got added in libsass |
|
@nschonni We tried upgrading last week, but as written above node-sass does not work anymore for our codebase at all and throws I wanted to try to create a repro case and open a new issue, but haven't had time to do so and might not have time for the next weeks. |
|
We are aware of the memory issue. A patch will landing the next LibSass 3.5.beta. The LibSass 3.5.beta will land in Node Sass 5 betas in the new year. |
|
Thanks @xzyfer. Just ran into this too. |
|
In case it helps anyone isolate or understand the bug, I made a code demo that reproduces the issue on my machine: https://github.com/benallfree/node-sass-1669 |
|
Is there a version of node-sass that we can downgrade to in the interim? I am also seeing this issue with: |
Would love to know this too =) |
|
@adampetrie @dbpolito 3.x works. edit: 3.14 as shown in https://github.com/benallfree/node-sass-1669 |
|
Those are def. two different issues here! See sass/libsass#2286 for an explanation and fix for the "stack too deep" error. Thanks @benallfree for the test case, finding this would have been much harder without 👍 Anybody knows with what "trickery" they acheive the concurrency? Not sure if you should expect other hard to detect and/or random errors when running this way. LibSass is not guaranteed to be thread safe! |
|
@mgreter If I'm coming to this issue from https://github.com/jtangelder/sass-loader#333 where I discovered it. @adampetrie See https://github.com/benallfree/node-sass-1669 for an example that works with |
|
@benallfree it's pretty clear what is happening, but not why. And yes, separating them in different processes will always work safely! Older node-sass work because this check was AFAIR added quite recently. sass/libsass#2286 should fix this issue, but I'm still a bit worried that there are other unknown side effects hidden in LibSass. I guess time will tell. Here's a simplified node-sass only reproduction of the second issue: var sass = require('node-sass');
function render() {
sass.render({
data: [
"@mixin test2() {",
" foo: bar;",
"}",
"@mixin test() {",
" @for $i from 1 to 100000 {",
" }",
"}",
"foo {",
" @include test2;",
" @include test;",
" @include test2;",
"}",
].join("\n"),
}, function(error, result) {
if (!result) console.log('error ', error);
});
}
for (var i = 0; i < 100; i ++) {
setTimeout(render, Math.random() * 1000);
}FWIW: The original issue reported here is just how LibSass handled memory. Not really a memory leak. This will improve drastically with my latest memory patches landing in LibSass. |
|
For others who come along, I can confirm that 3.14 works correctly too 😄 |
|
So it turns out I messed up the LibSass 3.4.2 release, and accidentally included the memory fixes scheduled for 3.5. Which means the memory fixes will should landing in the next node-sass minor this week. |
|
This should be fixed in 4.2.0. Please let me know how y'all go |
|
The recursion bug is fixed, but now we have random segmentation faults in random files. Not sure how I should report that. |
|
@danez do these segfault occur when using node-sass directly or via a build tool like grunt or gulp? I have seen this happen with grunt, but have been unable to reproduce whit node-sass directly. |
|
Yes they happen with But even if I change grunt-sass to do |
|
I thought that might be case but was unable to reproduce the issue using the same async loop extracted out from grunt. Could easily be timing sensitive though. |
|
I created #1861 |
|
@xzyfer - We've successfully worked around the problem by adding a "useRenderSync" option to grunt-sass which calls "renderSync" instead of "render". Haven't seen a single issue since we did that. Note that the fix isn't as simple as changing "render" to "renderSync", as the loop itself needs to be modified. This wasn't intended as a clean and nice fix, more of a quick and dirty hack to get it to work, but you can try it here. |
|
Did anyone find a solution for this? we have a memory leak issue & we suspect that it is related to this bug, since rubocop-thread_safety only shows the libsass within the node-sass to be the only offender for thread safety ... Is there a workaround at least to make the project thread safe? |
|
This issue was related to the memory problem @aelkoussy How are you testing node-sass with rubocop? |
|
@saper Actually I am not testing node-sass especially, I am testing my project using this cmd:
|
|
Giving output like this (I truncated the output) |
|
Those errors are reported because https://github.com/sass/libsass/blob/8057222005fbb9fc173f70b240befd25e15673db/contrib/libsass.spec file is an https://github.com/rpm-software-management/rpm RPM Package Manager file, not a Ruby file. It seems like you are running into an issue rubocop/rubocop#4553 which is not related to node-sass or libsass at all. |
|
I felt a bit weird about the files but due to that issue I thought it might be a cause for my memory problem, thanks for explaining @saper |
We are running constantly into out of memory errors with node-sass. Our scss setup is rather big and consists of 1076 scss files whereas 383 of them start with _.
We recently changed one of the scss underscore files to contain a map with all checksums for our images and this file is around 500kB big. What we did not expect is that node-sass/libsass memory usage will increase nearly by 100% which in our case means it jumped from 1.3GB to 2.1GB RAM usage.
All this memory is not freed after node-sass/libsass finished its work, and the watch tasks that run in the same node process after the full sass build still consumes > 2GB.
Even without our recent change 1.3GB is a massive amount of memory already and looks very much like memory leaks.
I profiled node-sass, but the profiler tells me that node only uses 30MB of memory, so the rest is probably coming from the native extension.
I'm not a c++ developer and do not know how to profile and debug the native extension. I could provide more details if anyone tells me how to get them. Unfortunately I cannot share our scss codebase.
version info:
The text was updated successfully, but these errors were encountered: