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

Fixes #37995 - High memory rendering Arf Report index page #584

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

hao-yu
Copy link
Member

@hao-yu hao-yu commented Nov 8, 2024

Avoid using includes() with nested associations and "order by" together. Otherwise, includes() will use join tables instead and Rails somehow create many objects and high memory consumption.

For more information, please refer to https://projects.theforeman.org/issues/37995

Avoid using includes() with nested associations and "order by"
together. Otherwise, includes() will use join tables instead
and Rails somehow create many objects and high memory
consumption.
@hao-yu hao-yu force-pushed the fixes_37995_arfreport_index_page branch from eab73eb to 5112cfc Compare November 11, 2024 03:19
@hao-yu hao-yu changed the title Fixes #37995 - Remove unneeded association Fixes #37995 - High memory rendering Arf Report index page Nov 11, 2024
@hao-yu
Copy link
Member Author

hao-yu commented Nov 11, 2024

Before patching:

# production.log
2024-11-11T03:30:46 [I|app|28125214] Started GET "/compliance/arf_reports?locale=en_GB&order=host+ASC&per_page=50&page=1"
2024-11-11T03:30:46 [I|app|28125214] Processing by ArfReportsController#index as HTML
2024-11-11T03:30:46 [I|app|28125214]   Parameters: {"locale"=>"en_GB", "order"=>"host ASC", "per_page"=>"50", "page"=>"1"}
2024-11-11T03:31:12 [I|app|28125214]   Rendered /usr/share/gems/gems/foreman_openscap-7.1.1/app/views/arf_reports/index.html.erb within layouts/application (Duration: 24621.6ms | Allocations: 5161641)
2024-11-11T03:31:12 [I|app|28125214]   Rendered layouts/base.html.erb (Duration: 107.0ms | Allocations: 38573)
2024-11-11T03:31:12 [I|app|28125214]   Rendered layout layouts/application.html.erb (Duration: 24755.6ms | Allocations: 5212234)
2024-11-11T03:31:12 [I|app|28125214] Completed 200 OK in 25678ms (Views: 18297.3ms | ActiveRecord: 6713.5ms | Allocations: 5306832

# ps
foreman   647039 16.8  6.9 1781540 1409588 ?     Sl   03:30   0:25 puma: cluster worker 0: 646810 [foreman]

VS

After patching

# production.log
2024-11-11T03:36:48 [I|app|e2d8b488] Started GET "/compliance/arf_reports?locale=en_GB&order=host+ASC&per_page=50&page=1"
2024-11-11T03:36:48 [I|app|e2d8b488] Processing by ArfReportsController#index as HTML
2024-11-11T03:36:48 [I|app|e2d8b488]   Parameters: {"locale"=>"en_GB", "order"=>"host ASC", "per_page"=>"50", "page"=>"1"}
2024-11-11T03:36:54 [I|app|e2d8b488]   Rendered /usr/share/gems/gems/foreman_openscap-7.1.1/app/views/arf_reports/index.html.erb within layouts/application (Duration: 3246.5ms | Allocations: 522083)
2024-11-11T03:36:54 [I|app|e2d8b488]   Rendered layouts/base.html.erb (Duration: 193.8ms | Allocations: 38579)
2024-11-11T03:36:54 [I|app|e2d8b488]   Rendered layout layouts/application.html.erb (Duration: 3471.1ms | Allocations: 572679)
2024-11-11T03:36:54 [I|app|e2d8b488] Completed 200 OK in 5825ms (Views: 3188.5ms | ActiveRecord: 609.0ms | Allocations: 830148)

# ps
foreman   647744  9.3  3.2 1035048 650132 ?	 Sl   03:35   0:13 puma: cluster worker 0: 647697 [foreman]

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @hao-yu !

Sorry for not checking this out earlier. It indeed reduces the count of allocations and drops rendering time. Also, I didn't find any drawbacks of this optimization: everything seems to be working still.

@adamruzicka, any objections?

Some observations unrelated to this PR: I wanted to test this approach in other places, but it seems we already follow the same rule: search and paginate at first, then use .inlcudes/.preload/etc. At least most of the time. There are some exceptions, such as policies in this plugin and the host page in the Foreman itself.

I've tried to make some presumably unnecessary small changes to hosts_controller.rb and users_controller.rb in Foreman and even with small set of entities I've noticed:

  • Hosts page without patch:
    • Completed 200 OK in 323ms (Views: 241.1ms | ActiveRecord: 40.2ms | Allocations: 210751)
  • Hosts page with patch:
    • Completed 200 OK in 305ms (Views: 195.0ms | ActiveRecord: 40.8ms | Allocations: 233857)
  • Users page with patch:
    • Completed 200 OK in 55ms (Views: 34.1ms | ActiveRecord: 3.8ms | Allocations: 57181)
  • Users page without patch:
    • Completed 200 OK in 74ms (Views: 47.7ms | ActiveRecord: 11.1ms | Allocations: 74892)

Even though it somehow increased the number of allocations for hosts page, it still reduced the time of the response... Doesn't seems right, but that was just poking around, not a real patch.

For users page though, it improved both numbers.

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I'm not exactly fond of the final sort being done in ruby, but we can live with that, considering it is just a page's worth of results. In the end, 5x improvement is rather nice.

There's on styling change that would be nice to have resolved before merging, but we can do that ourselves.

app/controllers/arf_reports_controller.rb Outdated Show resolved Hide resolved
@ofedoren
Copy link
Member

Thanks, @hao-yu and @adamruzicka !

@ofedoren ofedoren merged commit 60072b8 into theforeman:master Nov 27, 2024
24 of 26 checks passed
@hao-yu
Copy link
Member Author

hao-yu commented Nov 28, 2024

@foedoren maybe your patch would work nice for larger data but a bit of trade off in small data?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants