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

Independent Study. Creating use of bullet gem and .includes to speed up db accesses #2171

Closed
wants to merge 19 commits into from

Conversation

harshkachhadia
Copy link

@harshkachhadia harshkachhadia commented Dec 17, 2021

Project Description

  • This project focuses on increasing page load speed by reducing number of db queries by solving N+1 problem in rails.
  • N+1 problem is solved at various places, as seen in files changes in this project, by implementing eager loading with the help of 'includes' method.
  • 'Bullet' gem has been used to detect the location in expertiza code, where eager loading needs to be implemented using 'includes' method in order to reduce the number of queries made to database.

Link to Wiki

Team
Dr. Edward Gehringer(efg)(mentor)

  • Harsh Kachhadia (harshkachhadia)
  • Tirth Patel (tirthpatel7498)

@expertiza-travisci-bot
Copy link

expertiza-travisci-bot commented Dec 17, 2021

Hey @harshkachhadia,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

@expertiza-bot
Copy link
Contributor

1 Message
💬 Thanks for the pull request, and welcome! 🎉 The Expertiza team is excited to review your changes, and you should hear from us soon.

This repository is being automatically checked for code quality issues using Code Climate.
You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a pull request is considered ready to review.

If you have any questions, please send email to expertiza-support@lists.ncsu.edu.

UUID 3 Warnings
ecaa ⚠️ There are code changes, but no corresponding tests.
Please include tests if this PR introduces any modifications in behavior.
c951 ⚠️ You should commit changes to the DB schema (db/schema.rb) only if you have created new DB migrations.
Please double check your code. If you did not aim to change the DB, please revert the DB schema changes.
c51d ⚠️ You are modifying Gemfile or Gemfile.lock, please double check whether it is necessary.
You are suppose to add a new gem only if you have a very good reason. Try to use existing gems instead.
Please revert changes to Gemfile.lock made by the IDE.

Generated by expertiza-bot

@coveralls
Copy link

coveralls commented Dec 20, 2021

Coverage Status

Coverage remained the same at 39.31% when pulling 8b15480 on harshkachhadia:beta into eecd728 on expertiza:beta.

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.4%) to 31.886% when pulling 5901657 on harshkachhadia:beta into eecd728 on expertiza:beta.

Gemfile.lock Outdated
@@ -698,4 +703,4 @@ DEPENDENCIES
zip-zip

BUNDLED WITH
1.16.6
1.17.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

please run bundle with 1.16.6

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@@ -44,12 +44,14 @@ def self.get_user_list(user)
participants << course.get_participants
end
assignments = Assignment.where(instructor_id: user.id)
assignments.each do |assignment|
#Line 48 implements eager loading using 'includes' method. For detail, visit: http://tiny.cc/9aomuz
assignments.includes([:participants]).each do |assignment|
participants << assignment.participants
end
participants.each do |p_s|
Copy link
Collaborator

Choose a reason for hiding this comment

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

refactor p_s to participant. variable name is unclear

Copy link
Author

Choose a reason for hiding this comment

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

The only change we made in line 5 was to add

.includes([:participants]) + comment

other than this, all the code is as it was before. I am assuming it should be fine as it was already there in the beta branch before we even started working.

@@ -1,7 +1,8 @@
<%# this page will display the notifications as a flash message Randal Myers, 22 March 2017 %>

<% course_ids = [] %>
<% current_user.participants.each{|p| course_ids << p.assignment.course.id if p.assignment and p.assignment.course} %>
<%#Line 5 implements eager loading using 'includes' method. For detail, visit: http://tiny.cc/9aomuz%>
<% current_user.participants.includes(:assignment, assignment: [:course]).each{|p| course_ids << p.assignment.course.id if p.assignment and p.assignment.course} %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

use && instead of and

Copy link
Author

Choose a reason for hiding this comment

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

The only change we made in line 5 was to add

.includes(:assignment, assignment: [:course]) + comment

other than this, all the code is as it was before. I am assuming it should be fine as it was already there in the beta branch before we even started working.

Bullet.alert = true
Bullet.bullet_logger = true
Bullet.console = true
Bullet.rails_logger = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to be logging these when we merge this project in?

Copy link
Author

Choose a reason for hiding this comment

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

We won't need logging when we merge. I will turn all these to 'false'. This configuration is for 'bullet gem', which helps in detecting N+1 problems in the application using logging. This can be later turned to 'true' if someone wishes to find and resolve N+1 issues in future codebase.

Copy link
Collaborator

@johnbumgardner johnbumgardner left a comment

Choose a reason for hiding this comment

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

fix gems

no_proxy_fix (0.1.2)
nokogiri (1.9.1)
nokogiri (1.10.10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs to be usable with ruby version 2.2.7

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.

None yet

5 participants