Excessive memory use with many relations (>500) #694
Comments
|
Thanks for reporting this. A PR would be amazing. I think the first option should be better! |
|
Oh and please target release-5.3 branch :) |
|
The first option was my preference as well, although the same issue exists with the repositories although not as excessive, and I could not think of a better way of solving than the following which is consistent with option 2. https://gist.github.com/DangerDawson/cf62461fdb29acf58c77ce92a9fe213c As there is not a concept of a repository registry / finaliser which we could use to employ the same technique. I will put together a PR over the weekend and target the release-5.3 |
|
I also did some profiling around setting up of the relation readers for the repository to get a comparison of the effect of defining the readers once rather than per repository: BeforeCall Tree
Flame Graph
AfterCall Tree
Flame Graph
As you can see, we make a saving of 130Mb after we apply the PR for our code base |
|
I'm preparing 5.4 and it turned out this improvement breaks tests in my app. I rolled back the changes for now in release-5.4. I'll take another look once I have my tests green. |
|
Did you get to the bottom of what was the cause? |
|
@DangerDawson, not yet. My app has a sophisticated setup, and I also use a custom relation reader. If the latter is the issue, I hope it'll be just updating it to the new environment. But before looking into this, I'll release all the other gems prepared for 3.4. |
|
@DangerDawson good news, I figured this all out. I had a custom relation reader in my app so since its interface had changed I had to update it as well. It's no problem since it wasn't declared as a public interface anyway. I went ahead and refactored the new relation reader so that it's easier to customize in one's app. I'll release 5.4 today. Thank you so much for putting effort into it, it's a great improvement. |
|
@flash-gordon I have looked at your revised PR and it is a much better implementation, so thank you as well. |
|
@DangerDawson I'll deploy it later today and say if I see the difference on my coarse graphs |
Describe the bug
When booting with a lot of relations (we have 580) and profiling the setup of the container using vernier we see a lot of retained memory around the
registry_readerpluginhttps://github.com/rom-rb/rom/blob/v5.3.2/core/lib/rom/setup/finalize/finalize_relations.rb#L46
It looks as though the plugin used to setup the relation methods on each of the relations is causing the issue
Call Tree
Flame graph
After experimenting I can reduce this quite considerably over using one of two techniques (366Mb -> 155Mb)
Define the methods in a anonymous module and then include that into each relations by passing it into the plugin
https://gist.github.com/DangerDawson/0eb82f7e61cc791a31af120427f128ac
The same as 1. but use a class attribute to cache the anonymous module
https://gist.github.com/DangerDawson/46164ba54de58e8a697d2ecb9f36a02e
Outcome
Call Tree
Flame graph
I am happy to submit a patch, but wanted to get an idea of the preference of the two solutions.
I have also seen the same issue with the repositories as well, which take a similar approach to setup the relations for each repository, but I will follow up with that once I have had some feedback on the above
As a footnote, even with the patch does over 150mb or retained memory sound reasonable? I keep thinking we may have some references to some large objects which we are not freeing up after the relations are built
The text was updated successfully, but these errors were encountered: