search

rom-rb / rom

Data mapping and persistence toolkit for Ruby
https://rom-rb.org
MIT License
2.08k stars 161 forks source link

Excessive memory use with many relations (>500) #694

Closed DangerDawson closed 3 weeks ago

DangerDawson commented 1 month ago

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

image

Flame graph

image

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

image

Flame graph

image

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 commented 1 month ago

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

solnic commented 1 month ago

Oh and please target release-5.3 branch :)

DangerDawson commented 1 month ago

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 commented 1 month ago

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:

https://github.com/rom-rb/rom/pull/695/commits/a39ac5c7640ee5c5f27d70a3271c7d57ed3f58b3

Before

Call Tree

image

Flame Graph

image

After

Call Tree

image

Flame Graph

image

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