High-level caching for closet lists

Okay, so I still don't know why rendering is just so slow (though
migrating away from item translations did help!), but I can at least
cache entire closet lists as a basic measure.

That way, the first user to see the latest version of a closet list
will still need just as much time to load it… but *only* the ones that
have changed since last time (rather than always the full page), and
then subsequent users get to reuse it too!

Should help a lot for high-traffic lists, which incidentally are likely
to be the big ones belonging to highly active traders!

One big change we needed to make was to extract the `user-owns` and
`user-wants` classes (which we use for trade matches for *the user
viewing the list right now*) out of the cached HTML, and apply them
after with Javascript instead. I always dislike moving stuff to JS, but
the wins here seem. truly very very good, all things considered!
This commit is contained in:
Emi Matchu 2024-02-20 18:42:42 -08:00
parent 13b92b30d0
commit 583f3c712f
8 changed files with 113 additions and 55 deletions

View file

@ -7,7 +7,11 @@
function hangersInit() { function hangersInit() {
for (var i = 0; i < hangersInitCallbacks.length; i++) { for (var i = 0; i < hangersInitCallbacks.length; i++) {
try {
hangersInitCallbacks[i](); hangersInitCallbacks[i]();
} catch (error) {
console.error(error);
}
} }
} }
@ -58,6 +62,36 @@
hangersEl.toggleClass("comparing"); hangersEl.toggleClass("comparing");
}); });
// Read the item IDs of trade matches from the meta tags.
const ownedIds =
document
.querySelector("meta[name=trade-matches-owns]")
?.getAttribute("value")
?.split(",") ?? [];
const wantedIds =
document
.querySelector("meta[name=trade-matches-wants]")
?.getAttribute("value")
?.split(",") ?? [];
// Apply the `user-owns` and `user-wants` classes to the relevant entries.
// This both provides immediate visual feedback, and sets up "Compare with
// Your Items" to toggle to just them!
//
// NOTE: The motivation here is caching: this allows us to share a cache of
// the closet list contents across all users, without `user-owns` or
// `user-wants` classes for one specific user getting cached and reused.
const hangerEls = document.querySelectorAll("#closet-hangers .object");
for (const hangerEl of hangerEls) {
const itemId = hangerEl.getAttribute("data-item-id");
if (ownedIds.includes(itemId)) {
hangerEl.classList.add("user-owns");
}
if (wantedIds.includes(itemId)) {
hangerEl.classList.add("user-wants");
}
}
/* /*
Hanger forms Hanger forms

View file

@ -45,24 +45,24 @@ class ClosetHangersController < ApplicationController
visible_groups = @user.closet_hangers_groups_visible_to(@perspective_user) visible_groups = @user.closet_hangers_groups_visible_to(@perspective_user)
@unlisted_closet_hangers_by_owned = find_unlisted_closet_hangers_by_owned(visible_groups) @unlisted_closet_hangers_by_owned = find_unlisted_closet_hangers_by_owned(visible_groups)
items = [] @items = []
@closet_lists_by_owned.each do |owned, lists| @closet_lists_by_owned.each do |owned, lists|
lists.each do |list| lists.each do |list|
list.hangers.each do |hanger| list.hangers.each do |hanger|
items << hanger.item @items << hanger.item
end end
end end
end end
@unlisted_closet_hangers_by_owned.each do |owned, hangers| @unlisted_closet_hangers_by_owned.each do |owned, hangers|
hangers.each do |hanger| hangers.each do |hanger|
items << hanger.item @items << hanger.item
end end
end end
if @public_perspective && user_signed_in? if @public_perspective && user_signed_in?
current_user.assign_closeted_to_items!(items) current_user.assign_closeted_to_items!(@items)
end end
@campaign = Fundraising::Campaign.current @campaign = Fundraising::Campaign.current

View file

@ -1,17 +1,11 @@
require 'cgi' require 'cgi'
require 'digest'
module ClosetHangersHelper module ClosetHangersHelper
def closet_hangers_help_class(user) def closet_hangers_help_class(user)
'hidden' unless user.closet_hangers.empty? 'hidden' unless user.closet_hangers.empty?
end end
def closet_hanger_partial_class(hanger)
'object'.tap do |class_name|
class_name << ' user-owns' if hanger.item.owned?
class_name << ' user-wants' if hanger.item.wanted?
end
end
def send_neomail_url(neopets_username) def send_neomail_url(neopets_username)
"https://www.neopets.com/neomessages.phtml?type=send&recipient=#{CGI.escape neopets_username}" "https://www.neopets.com/neomessages.phtml?type=send&recipient=#{CGI.escape neopets_username}"
end end
@ -124,8 +118,8 @@ module ClosetHangersHelper
def render_closet_lists(lists) def render_closet_lists(lists)
if lists if lists
render :partial => 'closet_lists/closet_list', :collection => lists, render partial: 'closet_lists/closet_list', collection: lists,
:locals => {:show_controls => !public_perspective?} locals: {show_controls: !public_perspective?}
end end
end end
@ -140,6 +134,20 @@ module ClosetHangersHelper
hangers ? hangers.size : 0 hangers ? hangers.size : 0
end end
def unlisted_hangers_cache_key(owned)
# It'd be nice if this was a closet list like any other, instead of being a
# weird separate thing! Then we could use the same closet list cache key,
# in which adding/removing stuff in the list touches the simple updated_at.
# Instead, we encode the hashed list of hanger IDs and last updated_at into
# the key! (The digest is to prevent the cache key from getting obscenely
# large for long lists!)
hangers = @unlisted_closet_hangers_by_owned[owned] || []
ids = hangers.map(&:id).join(",")
last_updated_at = hangers.map(&:updated_at).max
["owned=#{owned}", Digest::MD5.hexdigest(ids), last_updated_at.to_i]
end
def closet_lists_group_name(subject, owned) def closet_lists_group_name(subject, owned)
ownership_key = owned ? 'owned_by' : 'wanted_by' ownership_key = owned ? 'owned_by' : 'wanted_by'
if subject == :you if subject == :you

View file

@ -1,6 +1,6 @@
class ClosetHanger < ApplicationRecord class ClosetHanger < ApplicationRecord
belongs_to :item belongs_to :item
belongs_to :list, class_name: 'ClosetList', optional: true belongs_to :list, class_name: 'ClosetList', optional: true, touch: true
belongs_to :user belongs_to :user
delegate :name, to: :item, prefix: true delegate :name, to: :item, prefix: true

View file

@ -1,4 +1,5 @@
%div{'class' => closet_hanger_partial_class(closet_hanger), 'data-item-id' => closet_hanger.item_id, 'data-quantity' => closet_hanger.quantity, 'data-id' => closet_hanger.id} -# NOTE: Changing this requires bumping the cache at `_closet_list.html.haml`!
.object{'data-item-id' => closet_hanger.item_id, 'data-quantity' => closet_hanger.quantity, 'data-id' => closet_hanger.id}
= render_item_link(closet_hanger.item) = render_item_link(closet_hanger.item)
.quantity{:class => "quantity-#{closet_hanger.quantity}"} .quantity{:class => "quantity-#{closet_hanger.quantity}"}
%span= closet_hanger.quantity %span= closet_hanger.quantity

View file

@ -108,13 +108,13 @@
%h4= t '.unlisted.header' %h4= t '.unlisted.header'
- if !@public_perspective - if !@public_perspective
= render partial: 'closet_lists/trading_neomail_warning', locals: {list: @user.null_closet_list(owned), user: @user} = render partial: 'closet_lists/trading_neomail_warning', locals: {list: @user.null_closet_list(owned), user: @user}
- cache unlisted_hangers_cache_key(owned) do
.closet-list-content .closet-list-content
.closet-list-hangers .closet-list-hangers
= render_unlisted_closet_hangers(owned) = render_unlisted_closet_hangers(owned)
%span.empty-list= t '.unlisted.empty' %span.empty-list= t '.unlisted.empty'
- if user_signed_in? - if user_signed_in?
- localized_cache action_suffix: 'tmpls' do
%script#autocomplete-item-tmpl{type: 'text/x-jquery-tmpl'} %script#autocomplete-item-tmpl{type: 'text/x-jquery-tmpl'}
%a %a
= t '.autocomplete.add_item_html', item_name: '${item_name}' = t '.autocomplete.add_item_html', item_name: '${item_name}'
@ -154,3 +154,14 @@
= include_javascript_libraries :jquery, :jquery_tmpl = include_javascript_libraries :jquery, :jquery_tmpl
= javascript_include_tag 'ajax_auth', 'lib/jquery.ui', 'lib/jquery.jgrowl', = javascript_include_tag 'ajax_auth', 'lib/jquery.ui', 'lib/jquery.jgrowl',
'closet_hangers/index' 'closet_hangers/index'
- if public_perspective? && user_signed_in?
- content_for :meta do
%meta{
name: "trade-matches-owns",
value: @items.select(&:owned?).map(&:id).join(",")
}
%meta{
name: "trade-matches-wants",
value: @items.select(&:wanted?).map(&:id).join(",")
}

View file

@ -18,6 +18,8 @@
- if show_controls - if show_controls
= render partial: 'closet_lists/trading_neomail_warning', locals: {list: closet_list, user: @user} = render partial: 'closet_lists/trading_neomail_warning', locals: {list: closet_list, user: @user}
-# Cachebuster comment: Updated downstream content at Feb 20 2024, 6:15pm
- cache closet_list do
.closet-list-content .closet-list-content
- if closet_list.description? - if closet_list.description?
= closet_list_description_format closet_list = closet_list_description_format closet_list

View file

@ -1,4 +1,6 @@
-# NOTE: Changing this requires bumping the cache at `_closet_list.html.haml`!
= link_to item_path(item) do = link_to item_path(item) do
= image_tag item.thumbnail_url, :alt => item.description, :title => item.description = image_tag item.thumbnail_url, alt: "Thumbnail for #{item.name}",
title: item.description, loading: "lazy"
%span.name= item.name %span.name= item.name
= nc_icon_for(item) = nc_icon_for(item)