search

jhiesey / stream-http

Streaming node http in the browser
MIT License
354 stars 62 forks source link

nested and on-the-fly functions removed, bind introduced instead #43

Open veljkopopovic opened 8 years ago

veljkopopovic commented 8 years ago

Using bind instead of 'on-the-fly' functions, self variable will not be trapped in a closure, allowing garbage collector to release resources once function is done.

On the other hand, 'on-the-fly' functions introduces a bit of performance penalty since engine is forced to compile function over and over again. This way, functions are statically created, no context variable dependency, arguments remain only function input parameter ...

jhiesey commented 8 years ago

How much does this improve memory usage in practice? I'd like to see some numbers before merging, especially since I don't personally like this stylistically as much.

veljkopopovic commented 8 years ago

Merge it or not, it's up to you. If you do some comparison, feel free to let me know, we have recorded leaks on SmartOs and this was a partial solution for that but we did not save any records about it. We do not have time to reproduce it right now.

Main reasons I have done it this way was to, first of all, remove so called calback hell and get a bit clearer picture of given function usage. On the other hand, but not less important, using self variable all around nested functions will not give a chance to a garbage collector to recognize moment when 'self' is not necessary any more, missing a chance to collect it and destroy it. This scenario will lead to memory leaks eventually. Some engines promises better support for garbage collecting inline functions, but promises are one thing and reality is another.

Best regards

On Sun, Jul 10, 2016 at 8:54 AM John Hiesey notifications@github.com wrote:

How much does this improve memory usage in practice? I'd like to see some numbers before merging, especially since I don't personally like this stylistically as much.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jhiesey/stream-http/pull/43#issuecomment-231574094, or mute the thread https://github.com/notifications/unsubscribe/ABdJanEFyi7DlQ7peCQSTzRDq0yvRnoEks5qUJcsgaJpZM4Iv5YY .

jhiesey commented 8 years ago

Sounds like it might help. I'll give it a test in a few modern browsers sometime in the near future and let you know how it goes. Thanks!

jscheid commented 7 years ago

I think this would also help with a Google Closure Compiler error I'm running into:

stdin:245570: ERROR - functions can only be declared at top level or immediately within another function in ES5 strict mode
        function read () {
        ^^^^^^^^^^^^^^^^^^

(The filename/line number are cryptic, but I've traced them back to https://github.com/jhiesey/stream-http/blob/v2.7.2/lib/response.js#L46)

So, I'd like to put in a vote for getting this merged (after conflicts are fixed) :-)

jhiesey commented 7 years ago

OK, I'll take another look into this week and see about fixing the conflicts.