Excessive memory use with many relations (>500)

[DangerDawson]

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_reader plugin

https://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)

1. 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

2. 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

[solnic]

Thanks for reporting this. A PR would be amazing. I think the first option should be better!

[solnic]

Oh and please target release-5.3 branch :)

[DangerDawson]

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

[DangerDawson]

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:

a39ac5c

Before

Call Tree

Flame Graph

After

Call Tree

Flame Graph

As you can see, we make a saving of 130Mb after we apply the PR for our code base

[flash-gordon]

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.

[DangerDawson]

Did you get to the bottom of what was the cause?

[flash-gordon]

@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.

[flash-gordon]

@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.

[DangerDawson]

@flash-gordon I have looked at your revised PR and it is a much better implementation, so thank you as well.
Out of curiosity did you see any speedup / memory reductions like we did?

[flash-gordon]

@DangerDawson I'll deploy it later today and say if I see the difference on my coarse graphs