rubyvideo icon indicating copy to clipboard operation
rubyvideo copied to clipboard

Enhance featured events logic and update event associations

Open nageswarchedella opened this issue 4 months ago • 3 comments

This pull request addresses a performance issue on the home page by eliminating several N+1 queries that were slowing down page load times.

Problem

The home page was making individual database calls for each featured event to fetch its details. This created a classic N+1 query problem, where the number of queries scaled with the number of featured events, leading to poor performance.

Fixes issue: #950

Solution

The code has been refactored to load all necessary data in a more efficient way:

  1. Pre-loading Event Records: In PageController, instead of just fetching playlist slugs, we now fetch the Static::Playlist objects and then pre-load their corresponding Event records in a single, efficient query.
  2. Eager-Loading Associations: The @featured_events query now eager-loads all required associations (:organisation, :speakers, :talks, :keynote_speakers) to prevent further N+1 queries in the view layer.
  3. Refactored keynote_speakers association: The keynote_speakers association on the Event model has been optimized to use a more direct keynote_talks association, improving the performance of fetching keynote speakers.

Verification:

To ensure these changes are safe and do not break existing functionality, a new test has been added to test/controllers/page_controller_test.rb.

  • test "home page should render featured events": This test confirms that the home page renders successfully and that the "Latest talks" section is present.

The full Rails test suite (244 tests) is now passing, providing strong confidence that these optimizations do not introduce any regressions.

(Note: The main bin/ci script fails due to a pre-existing, unrelated issue in the bundler-audit step.)

Load Test results

I have performance load testing with latest changes from the main branch and current branch by using wrk tool. The test results from my local desktop are as follows:

Before:

$  wrk -t2 -c10 -d20s http://localhost:3000/ --timeout 30s
Running 20s test @ http://localhost:3000/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.94s     2.50s   10.03s    80.36%
    Req/Sec     4.75      2.56    20.00     78.63%
  186 requests in 20.08s, 25.26MB read
Requests/sec:      9.26
Transfer/sec:      1.26MB

After:

$  wrk -t2 -c10 -d20s http://localhost:3000/ --timeout 30s
Running 20s test @ http://localhost:3000/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.83s     2.38s    9.61s    80.00%
    Req/Sec     5.21      2.65    19.00     79.09%
  192 requests in 20.09s, 26.08MB read
Requests/sec:      9.56
Transfer/sec:      1.30MB

nageswarchedella avatar Aug 29 '25 18:08 nageswarchedella

Thanks for this PR. I rebased main locally and push it to prod to test it in real life (the deploy is right in the middle of the chart

68c7121de8f1f033c6d05238

It leads to a small bump in response time. I think as @hschne mentioned maybe it is best to have from time to time an N+1 rather than always sa more complex query

From this PR I think we could keep the has_many :keynote_talks and has_many :keynote_speakers association update and the controller test

adrienpoly avatar Sep 14 '25 19:09 adrienpoly

It leads to a small bump in response time. I think as @hschne mentioned maybe it is best to have from time to time an N+1 rather than always as more complex query

If it increases the response time, I would not recommend it either.

From this PR I think we could keep the has_many :keynote_talks and has_many :keynote_speakers association update and the controller test

I will make necessary changes as you suggested.

nageswarchedella avatar Sep 16 '25 17:09 nageswarchedella

Hi @adrienpoly , I have updated the PR with the recommended changes. Could you please have a look at it?

nageswarchedella avatar Sep 17 '25 18:09 nageswarchedella