Fix Some Leaks #2128

stefanpenner · 2017-10-31T18:12:53Z

[FIXES #2098]

We noticed our ember-cli builds for linkedin.com were consuming ~10gb of memory, after some investigation node-sass (via eyeglass) was identified as the primary culprit. With this PR, memory usage dropped by from ~10gb -> ~1gb (saving about 9gb of ram)

NOTE: This does not fix all the leaks, more PR's to come

Fixes so far:

create_string must be paired with a free (this catches a few of the easy ones)
- number constructor's unit (sass_make_number copies the unit)
- string constructor's value (sass_make_string also copies the string)
- Bindings ExtractOptions function signature (sass_make_function copies the signature)
SassTypes::Value are now subclasses of Nan::ObjectWrap, and we utilize its machinery for memory management. This ensures our wrapper objects no longer leak Nan::ObjectWrap docs

Tests:

add some isolated memory-test examples, which are handy to diagnose/fix memory leaks
back-port tests for sass.types.*
in sass-eyeglass/eyeglass
in the build system for Linkedin.com

Notes:

I realize this PR is quite large, the only drastic change was utilizing Nan::ObjectWrap memory management machinery. The rest is a mixture of tests, and cleanup to simplify the memory issues.

I look forward to you code reviews!

stefanpenner · 2017-10-31T18:33:19Z

Looks like i got some eslint errors, will circle back shortly and fix.

saper · 2017-10-31T19:11:06Z

Looks good to me, only need to wade through white space changes....

stefanpenner · 2017-10-31T21:12:19Z

only need to wade through white space changes....

sorry about that, I can likely split the whitespace stuff into a separate commit tonight if you need.

kyleshay · 2017-10-31T21:15:23Z

FYI the w=1 query param hides whitespace changes:
https://github.com/sass/node-sass/pull/2128/files?w=1

xzyfer · 2017-11-01T13:01:43Z

Thanks @stefanpenner, this all looks great to me. I'll investigate what's up with CI, it failure looks to be transient.

stefanpenner · 2017-11-01T15:22:18Z

@xzyfer / @saper thanks.

If you got some time to chat let me know. I have some ideas that might improve this bridge code some more, but would be a larger change, and I wouldn’t want to go down that path unless it sounds semi ok to you folks.

stefanpenner · 2017-11-01T21:18:29Z

@xzyfer I appears the failure is on node 9, its possible that #2130 may address? Will investigate locally

xzyfer · 2017-11-02T12:14:25Z

I see. We test against latest which has since been updates to Node 9 before we were ready.

We're definitely open to talking more about the binding code. Most of it predates myself, and possibly all the active currently team. You can join our Slack to chat (timezones permitting).

* boolean * function-bridge * map * string

Nan::ObjectWrap Docs: https://github.com/nodejs/nan/blob/master/doc/object_wrappers.md#api_nan_object_wrap Prior to this, all wrapper Objects would never be deleted, allowing memory to grow unbounded. Example: The following, would leak WrapperObjects, and never ``` while (true) { new sass.types.String(‘I LEAK’); } ```

stefanpenner · 2017-11-02T20:23:14Z

rebased \w node 9 appveyor fix... let's see if CI agrees 🤞

stefanpenner · 2017-11-02T20:39:05Z

@xzyfer if this is green, do you think we can get a release out soon? If I can help at all with that, please don't hesitate to reach out.

xzyfer · 2017-11-02T21:06:11Z

We'll have to rebuild our binaries once this is merged which can take a while. We'll do a release with node 9 support first because that only requires building a binary for one version of node. This will fast follow.

…

On 3 Nov. 2017 7:39 am, "Stefan Penner" ***@***.***> wrote: @xzyfer <https://github.com/xzyfer> if this is green, do you think we can get a release out soon? If I can help at all with that, please don't hesitate to reach out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWEs817uMjAPEEvvKaWAK7dl-2wJ8ks5syihqgaJpZM4QNMa1> .

stefanpenner · 2017-11-02T21:19:12Z

@xzyfer can you guestimate an ETA (1 day, after the weekend, 1 week, 1 month etc).

I have to coordinate some stuff on my end of the world if this is a ways out yet. Knowing the above would help me plan :)

🍻

xzyfer · 2017-11-02T22:10:58Z

Either in the next 12hrs or when I get back from holidays in a week.

…

On 3 Nov. 2017 8:19 am, "Stefan Penner" ***@***.***> wrote: @xzyfer <https://github.com/xzyfer> can you guestimate an ETA (1 day, after the weekend, 1 week, 1 month etc). I have to coordinate some stuff on my end of the world if this is a ways out yet. This would help me out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWEOKK1qswADizhA-8aD1Pnl92m6lks5syjHQgaJpZM4QNMa1> .

stefanpenner · 2017-11-02T23:24:45Z

@xzyfer awesome, thanks for the heads-up!

stefanpenner · 2017-11-03T00:44:05Z

its green!

saper · 2017-11-03T23:28:08Z

src/custom_function_bridge.cpp

+    Sass_Value* x = static_cast<Sass_Value*>(value);
+    SassTypes::Value* y = SassTypes::Factory::create(x);
+
+    argv.push_back(y->get_js_object());


is this only cosmetic change or is there some deeper reason behind it?

saper · 2017-11-09T21:50:05Z

Merged with 734c7c8, thank you very much!

nschonni · 2017-11-09T21:56:52Z

@saper I think we should revert this (temporarily) and land #2147 as a 4.6.1. Including this means we need to rebuild every binary

stefanpenner · 2017-11-09T22:18:24Z

Merged with 734c7c8, thank you very much!

@saper I will likely have some more leak fixes soonish (maybe a week), but these address the largest of the leaks I've run into. It took our build from 10gb ram -> 1.3gb ram.

xzyfer · 2017-11-09T22:19:48Z

I agree with @nschonni. I'm also away atm so we can't build the new binaries.

saper · 2017-11-09T22:31:51Z

Temporarily reopened

saper · 2017-11-09T22:32:15Z

Shall we cut v4.6.1 now with #2147 ?

xzyfer · 2017-11-09T22:36:01Z

I have some questions about #2147. I'm not sure it's ready.

evanfuture · 2017-11-10T06:18:25Z

Sorry for the error on my part, but I think #2147 is good to go now. (also sorry, I'm in Europe, so we aren't in front of our screens at the same time!)

saper · 2017-11-10T07:37:14Z

Some of us are in Europe, too :) thanks

stefanpenner · 2017-11-10T16:22:51Z

Just sharing some positive test results:

We have been running with ^ for a few days now, Thousands of builds, across various node versions and MacOS Sierra, RHEL 6, RHEL 7. No issues and just (as predicted) a nice drop in RAM usage. We have seen as much as 22gb reduction in memory usage...

xzyfer · 2017-11-11T00:29:27Z

I'm aiming to land this in the next 48hrs. I'm actually hanging out with Chris this weekend. He says you have ideas on how to improve the binding layer. Feel free to join us our slack to discuss further. There's a slack badge in the read me.

…

On 11 Nov. 2017 3:22 am, "Stefan Penner" ***@***.***> wrote: Just sharing some positive test results: We have been running with ^ for a few days now, Thousands of builds, across various node versions and MacOS Sierra, RHEL 6, RHEL 7. No issues and just (as predicted) a nice drop in RAM usage. We have seen as much as 22gb reduction in memory usage... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWPIUet53Q2EuOcoa0geNzbfVKdIRks5s1HhcgaJpZM4QNMa1> .

xzyfer · 2017-11-11T23:28:16Z

Since we need to rebuild binaries for this I'm also going to bump libsass to that latest 3.5 release to fix a couple other outstanding issues.

saper · 2017-11-11T23:39:58Z

Great. TIL: using git commit ----cleanup=whitespace to add release notes in Markdown to the release commit - this will be picked up by @appveyor and added to the release tag on GitHub.

xzyfer · 2017-11-12T00:58:55Z

Cool! I'll tag a new libsass release in the next 24hrs. We'll jump straight to that version.

…

On 12 Nov. 2017 10:40 am, "Marcin Cieślak" ***@***.***> wrote: Great. TIL: using git commit ----cleanup=whitespace to add release notes in Markdown to the release commit <6b7b679> - this will be picked up <https://github.com/sass/node-sass/blob/6b7b67901bc7c1f803073089ad93b95ec5fc61d4/appveyor.yml#L80> by @appveyor <https://github.com/appveyor> and added to the release tag on GitHub. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWPmphJiv-mgA4ZRQrjB4Zh8oxrH3ks5s1jBOgaJpZM4QNMa1> .

stefanpenner force-pushed the leaks branch from 3eb1106 to 41550fd Compare October 31, 2017 18:24

stefanpenner force-pushed the leaks branch 2 times, most recently from 668499e to fb79165 Compare October 31, 2017 19:05

stefanpenner mentioned this pull request Nov 2, 2017

Node 9 support #2130

Merged

stefanpenner added 5 commits November 2, 2017 13:21

Add Isolated Memory Leak Scenarios

f2a09eb

* boolean * function-bridge * map * string

Backfill some test for sass.types.*

930c2d9

Remove dangling console.log from tests

4cf43ac

[LEAK FIX] create_string must be paired with a free

3052263

stefanpenner force-pushed the leaks branch from fb79165 to f6b0c72 Compare November 2, 2017 20:22

saper reviewed Nov 3, 2017

View reviewed changes

saper closed this Nov 9, 2017

saper reopened this Nov 9, 2017

xzyfer merged commit e3ca075 into sass:master Nov 11, 2017

stefanpenner deleted the leaks branch November 14, 2017 15:54

Vinnl mentioned this pull request Nov 28, 2017

Sementation fault since node-sass 4.7.1 #2169

Closed

dependabot-preview bot mentioned this pull request Mar 19, 2018

Bump node-sass from 4.5.3 to 4.7.2 fcknutwil/sponsorencrm_client#54

Closed

xzyfer mentioned this pull request Jul 8, 2018

Convert binding to N-API #2440

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Some Leaks #2128

Fix Some Leaks #2128

stefanpenner commented Oct 31, 2017 •

edited

Loading

stefanpenner commented Oct 31, 2017

saper commented Oct 31, 2017

stefanpenner commented Oct 31, 2017

kyleshay commented Oct 31, 2017

xzyfer commented Nov 1, 2017

stefanpenner commented Nov 1, 2017 •

edited

Loading

stefanpenner commented Nov 1, 2017

xzyfer commented Nov 2, 2017

stefanpenner commented Nov 2, 2017 •

edited

Loading

stefanpenner commented Nov 2, 2017

xzyfer commented Nov 2, 2017 via email

stefanpenner commented Nov 2, 2017 •

edited

Loading

xzyfer commented Nov 2, 2017 via email

stefanpenner commented Nov 2, 2017

stefanpenner commented Nov 3, 2017

saper Nov 3, 2017

stefanpenner Nov 8, 2017

saper commented Nov 9, 2017

nschonni commented Nov 9, 2017

stefanpenner commented Nov 9, 2017 •

edited

Loading

xzyfer commented Nov 9, 2017

saper commented Nov 9, 2017

saper commented Nov 9, 2017

xzyfer commented Nov 9, 2017

evanfuture commented Nov 10, 2017 •

edited

Loading

saper commented Nov 10, 2017

stefanpenner commented Nov 10, 2017

xzyfer commented Nov 11, 2017 via email

xzyfer commented Nov 11, 2017

saper commented Nov 11, 2017

xzyfer commented Nov 12, 2017 via email

Fix Some Leaks #2128

Fix Some Leaks #2128

Conversation

stefanpenner commented Oct 31, 2017 • edited Loading

Fixes so far:

Tests:

Notes:

stefanpenner commented Oct 31, 2017

saper commented Oct 31, 2017

stefanpenner commented Oct 31, 2017

kyleshay commented Oct 31, 2017

xzyfer commented Nov 1, 2017

stefanpenner commented Nov 1, 2017 • edited Loading

stefanpenner commented Nov 1, 2017

xzyfer commented Nov 2, 2017

stefanpenner commented Nov 2, 2017 • edited Loading

stefanpenner commented Nov 2, 2017

xzyfer commented Nov 2, 2017 via email

stefanpenner commented Nov 2, 2017 • edited Loading

xzyfer commented Nov 2, 2017 via email

stefanpenner commented Nov 2, 2017

stefanpenner commented Nov 3, 2017

saper Nov 3, 2017

Choose a reason for hiding this comment

stefanpenner Nov 8, 2017

Choose a reason for hiding this comment

saper commented Nov 9, 2017

nschonni commented Nov 9, 2017

stefanpenner commented Nov 9, 2017 • edited Loading

xzyfer commented Nov 9, 2017

saper commented Nov 9, 2017

saper commented Nov 9, 2017

xzyfer commented Nov 9, 2017

evanfuture commented Nov 10, 2017 • edited Loading

saper commented Nov 10, 2017

stefanpenner commented Nov 10, 2017

xzyfer commented Nov 11, 2017 via email

xzyfer commented Nov 11, 2017

saper commented Nov 11, 2017

xzyfer commented Nov 12, 2017 via email

stefanpenner commented Oct 31, 2017 •

edited

Loading

stefanpenner commented Nov 1, 2017 •

edited

Loading

stefanpenner commented Nov 2, 2017 •

edited

Loading

stefanpenner commented Nov 2, 2017 •

edited

Loading

stefanpenner commented Nov 9, 2017 •

edited

Loading

evanfuture commented Nov 10, 2017 •

edited

Loading