Significantly reduce load time on the Show page for large datasets #1989
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… line but rather pushing the calculation to happend in the AJAX call for the file list.
…and deletes by using a binary search rather than a plain include
hectorcorrea
changed the title
hectorcorrea
commented
Nov 20, 2024
| # can consume. The `data` elements includes the work's file list all other | ||
| # properties are used for displaying different data elements related but not | ||
| # directly on the DataTable object (e.g. the total file size) | ||
| def file_list_ajax_response(work) |
There was a problem hiding this comment.
Notice how we return the file list plus a bunch of other values that depend on the file list
hectorcorrea
commented
Nov 20, 2024
| // The JSON payload includes a few extra properties (fetched via AJAX because | ||
| // they be slow for large datasets). Here we pick up those properties and update | ||
| // the display with them. | ||
| $("#total-file-size").text(json.total_size_display); |
There was a problem hiding this comment.
We update the page with the extra values that now come on the AJAX response
hectorcorrea
commented
Nov 20, 2024
| # much faster when the list of files is large. Notice that the binary search | ||
| # requires that the list of files is sorted. | ||
| # See https://ruby-doc.org/3.3.6/bsearch_rdoc.html | ||
| if s3_filenames_sorted.bsearch { |s3_filename| filename <=> s3_filename }.nil? |
There was a problem hiding this comment.
This is the other optimization, notice the use of Binary Search bsearch instead of a normal include?
hectorcorrea
commented
Nov 20, 2024
|
|
||
| def log_performance(start, message) | ||
| elapsed = Time.zone.now - start | ||
| if elapsed > 20 |
There was a problem hiding this comment.
We can tweak the threshold, but I thought it would be good to start logging as warning slow requests
| { | ||
| data: files, | ||
| total_size:, | ||
| total_size_display: ActiveSupport::NumberHelper.number_to_human_size(total_size), |
| # much faster when the list of files is large. Notice that the binary search | ||
| # requires that the list of files is sorted. | ||
| # See https://ruby-doc.org/3.3.6/bsearch_rdoc.html | ||
| if s3_filenames_sorted.bsearch { |s3_filename| filename <=> s3_filename }.nil? |
| # Calculates the total file size from a given list of files | ||
| # This is so that we don't fetch the list twice from AWS since it can be expensive when | ||
| # there are thousands of files on the work. | ||
| def total_file_size_from_list(files) |
jrgriffiniii
approved these changes
Nov 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Although we already loaded the file list via AJAX on the Show page, we still had several calls to fetch the file list as we rendered the page (e.g. to calculate and show the total file size.) For large works (e.g. works with 60K files) this calculation takes 30+ seconds which mean the page took 30+ seconds to load to the user.
In this PR I moved all the code that depends on the file list to display those values after the page has been rendered and the file list has been fetched (via AJAX).
While troubleshooting this issue I also noticed that our code to detect changes in the Upload Snapshots was incredibly slow on this kind of large datasets. Comparing 60,000 files 60,000 times for deletes + adds/modifications was very slow. Even thought this work is done in the background our implementation would take minutes to complete. So I updated the code that calculates the deletes + adds/modifications to use a binary search (instead of a regular Array
include?) and that improved performance significantly (3-4 seconds instead of 1+ minute).Closes #1623
This has been deployed so Staging and it can be tested with this work that has 40K files: https://pdc-describe-staging.princeton.edu/describe/works/650