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

Ideas to improve memory usage #558

Open
blikblum opened this issue Jan 31, 2016 · 9 comments
Open

Ideas to improve memory usage #558

blikblum opened this issue Jan 31, 2016 · 9 comments

Comments

@blikblum
Copy link
Contributor

Currently Rivets, even after the memory leaks fixes, uses a lot of memory, something that hurts specially in mobile and when used with other libraries like Backbone/Marionette.

Here are some ideas. Most of them are backward incompatible

  1. Do not observe functions passed as events handlers in on-* binder. I can't think in a good reason to change methods dynamically
  2. Do not copy properties from parent scope to child scope. This is the method used to access properties in parent scope now. One side effect is that the scope passed in event handler will not have the parent properties attached to it
  3. Remove the ability to configure adapters, binders per view. Currently every view has a copy of the adapters, binders, components and formatters. The two later has some reasoning to be configured per view, while the others not.
@benadamstyles
Copy link
Collaborator

I like 1 a lot, I think that's a great idea. But unless I'm missing something, would 2 and 3 make a lot of difference? Aren't they copied by reference not value? In other words, the copies are pointers, not clones, right?

@blikblum
Copy link
Contributor Author

Each key is copied to avoid clobbering the global definitions.

For a simple application a lot of view instances, child scopes are created

@jccazeaux
Copy link
Contributor

For 1: you would not observe the function, but will the path to the function be observed?
In this example

<button rv-on-click='model.currentCustomer.sendNotice'>Send notice</button>

The sendNotice part has no reason to change, but currentCustomer could change. Will it still work?

For 2 i had same reaction as @Leeds-eBooks. It's mainly pointers, does it really affect memory that much?

@stalniy
Copy link
Contributor

stalniy commented Feb 3, 2016

we need to take each binding and test it using developer tools and 3 times "Take Snapshot" approach, remove the nodes and check if rivets cleans up the views and bindings of the remove node. I believe this is what currently doesn't work properly.

@Duder-onomy
Copy link
Collaborator

@blikblum Do you think you can champion number 1 for the 0.9 release? If so, lets add it to the todo list.

After the release we can look into memory more and see if the other ideas truly warrant effort?

@benadamstyles
Copy link
Collaborator

I actually needed to change an rv-on-* function at runtime in my work yesterday. So maybe we should leave this as-is. My use case was that I needed to change what happened when someone clicked on a button, dependent on what options they chose elsewhere on the page. Although I could have simply queried the state of the option within the rv-on-* handler and performed the corresponding action, in this case the obvious choice was to swap out the function for a different one. Maybe this is bad code on my part though, and we should discourage it in rivets. Do you think the observation of functions causes a lot of memory overhead? If not, I can see how removing this would lead to a lot of Why is my function not updating? github issues. However, with good docs, we could probably get away with this if you think it will bring a noticeable improvement.

@Duder-onomy
Copy link
Collaborator

@Leeds-eBooks Ya know. I keep going back and forth how I feel about these backwards incompatible changes. I have really never had any issues expressing my needs in rivets (except when I was a padawan) and have never had any use case for changing the bound event handler. BUT, isnt this what makes Rivets awesome! That people can pretty much do what ever they want and it will probably work?

It blows my mind the diff ways people are using this and not seeing any problems.

@blikblum @stalniy @jccazeaux Do you really feel like memory is a issue right now? I mean, if there is a memory LEAK, we should kill it. But making backwards incompatible changes to make minor improvements?

I would like to see exactly how much memory this saves. If it is a big improvement. Then I will be for it. We use rivets on smart tvs, and memory is a HUGE problem for us.

@blikblum
Copy link
Contributor Author

blikblum commented Feb 4, 2016

@Duder-onomy the memory leaks were killed with the recent changes i submitted so the main memory issue is resolved right now (BTW: one big reason to release a new version ASAP).

The proposed changes here aims to improve memory usage and thus general performance but is not something essential like is the memory leaks.

IMO rivets uses much memory relatively to the features it provides. Most of the times, it is used with other libraries (in my case Marionette) which carry its own memory requirements. In desktop there's not much problem but in mobile it can make difference.

I don't know precisely how much memory those changes will save but i'm confident that will make a significant difference specially when each binder is used.

There still other change that would improve performance in rivets apps. Is changing the context of the called function. Currently is not consistent leading to the developer bind the methods for each instance (see https://github.com/tastejs/todomvc/pull/1295/files#diff-f101b80f5e1e2f6bc3ebdaaf68785be0R7). But this is an even bigger change.

All in all, i understand the reasons those changes not being implemented in rivets.

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

No branches or pull requests

5 participants