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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

veljkopopovic
Copy link

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
Copy link
Owner

jhiesey commented Jul 10, 2016

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
Copy link
Author

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
#43 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABdJanEFyi7DlQ7peCQSTzRDq0yvRnoEks5qUJcsgaJpZM4Iv5YY
.

@jhiesey
Copy link
Owner

jhiesey commented Jul 14, 2016

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
Copy link

jscheid commented Jul 13, 2017

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
Copy link
Owner

jhiesey commented Jul 19, 2017

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

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

Successfully merging this pull request may close these issues.

None yet

3 participants