-
Notifications
You must be signed in to change notification settings - Fork 36
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
[PERFORMANCE] [MER-4330] [MER-4329] Dashboard optimizations round2 #5442
Conversation
|
||
%{1: "Unit 1: Basics", 15: nil, 45: "Module 3: Enumerables"} | ||
""" | ||
def map_resources_with_container_labels(section_slug, resource_ids) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the second query that was run to load each tab. We no longer need it as we can glean the same information from SectionResourceDepot
@@ -60,10 +60,8 @@ defmodule OliWeb.Delivery.InstructorDashboard.Helpers do | |||
end | |||
|
|||
def get_assessments(section, students) do | |||
graded_pages_and_section_resources = | |||
DeliveryResolver.graded_pages_revisions_and_section_resources(section.slug) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first query that is run - a "expensive" resolver query to fetch Revisions + SectionResource records.
defp return_page(graded_pages_and_section_resources, section, _students) do | ||
# Create a map of all section resource ids to their parent container labels | ||
container_labels = | ||
Oli.Delivery.Sections.SectionResourceDepot.containers(section.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a map of section resource ids to their parent label containers. No DB queries here, just a Depot call.
send(pid, {:assessments, result_with_metrics}) | ||
end) | ||
|
||
result | ||
end) | ||
|> assign_new(:activities, fn -> Oli.Activities.list_activity_registrations() end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the key to the "perceived" faster loading of the views. We asynchronously run the three metrics queries, allowing the view to load very quickly, and then have the details fill in when they arrive.
…tiative/oli-torus into dashboard-optimizations-round2
This is round 2 of the Instructor Dashboard optimization. The work applies to the Scored Activities, Practice and Survey tabs.
Minimizes memory usage.
All three tabs in the dashboard were storing the full Revision plus the Section Resource record for all pages in their display. The solution here was to move to simply storing Section Record records, eliminating the Revision and its heavy
:content
field from being stored in process memory.Testing with an ingested Chem 2 course (with no student data) shows the Instructor Dashboard running at
2 MB
per instance. With this PR that is cut down to466 KB
Improve Dashboard loading time.
Previously, two queries executed to fetch and label the pages, and then three queries executed to load their metrics. All five of these queries had to run and finish before we can render the view.
This PR eliminates the first two queries by simply replacing them with
SectionResourceDepot
calls (one query had to be introduced for Surveys, but it should have low overhead as it only returns page ids). The three metrics queries are then asynchronously executed, allowing the view to display very quickly, and then fill in the metrics when those queries finish.Tech Debt
Along the way I discovered the LiveView unit tests of these three views were dense and detailed and extremely difficult to work with. It took me maybe 30 minutes to code and interactively test to verify the actual work of this PR, but then I found that it broke 30 some tests. I then spent a couple of hours attempting to fix these tests, but finally decided to just nuke them. They aren't that helpful and in some respects represent an anti-pattern - as they were UI based tests that were attempting to verify non UI tasks (which are already verified in other unit tests).