Matrix box carousels

[nimmolo]

Check it out. Rudimentary!

[pellaea]

That makes sense, yeah, it's a lot. Definitely worth checking how this affects performance!

…

On Thu, Sep 7, 2023 at 12:16 AM andrew nimmo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/presenters/matrix_box_presenter.rb <#1598 (comment)> : > if target&.respond_to?(:thumb_image) && target&.thumb_image self.image_data = { - image: target.thumb_image, - image_link: target.show_link_args, + images: target.images, Quite a bit more loading. It now has to be { images: [:image_votes, :license, :user] } — Reply to this email directly, view it on GitHub <#1598 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNOOPIKSDPHEBBHR6PDXZFDCXANCNFSM6AAAAAA4OHS4KY> . You are receiving this because your review was requested.Message ID: ***@***.*** com>

[nimmolo]

In terms of manual testing, please make sure the "hot" areas of the matrix_box images make sense and are usable.

There are quite a few of them for such a small area.

• fullscreen
• left/right carousel nav
• image votes
• obs_link (everywhere not covered by the above)
  (had experimented with leaving this one off the matrix_box carousels)

It would also be nice to benchmark the load time for this page vs. main. Volunteers encouraged!

[JoeCohen]

Is obs_link supposed to be part of the image?
(In my manual testing, only fullscreen, left/right carousel nav, and image votes are hot areas in the image. The consensus id and the obs id link to the image. I'm OK with doing it that way.)

Chrome version 116.0.5845.179 (Official Build) (x86_64)

vagrant@ubuntu-focal:/vagrant/mushroom-observer$ git log -1
commit a1eb3c639a42b24f4e70f6f4584be715bc609c52 (HEAD -> nimmo-matrix-box-carousels, origin/nimmo-matrix-box-carousels)
Merge: 237e2d533 d4d57f962
Author: andrew nimmo <andrnimm@fastmail.fm>
Date:   Thu Sep 7 13:19:48 2023 -0700

    Merge branch 'main' into nimmo-matrix-box-carousels

In terms of manual testing, please make sure the "hot" areas of the matrix_box images make sense and are usable.

There are quite a few of them for such a small area.

• fullscreen
• left/right carousel nav
• image votes
• obs_link (everywhere not covered by the above)

It would also be nice to benchmark the load time for this page vs. main. Volunteers encouraged!

[nimmolo]

@JoeCohen - you're right, the obs_link is not on the carousels. I agree, it's ok this way.

[nimmolo]

@JoeCohen ... aha, i figured out where I'd experimentally disabled it. The obs_link is back working now.

If you have time to check it out, please let me know if you think that's better or not. I do feel like people expect it at this point, but maybe not.

[coveralls]

coverage: 93.631% (-0.004%) from 93.635% when pulling c42ecc5 on nimmo-matrix-box-carousels into 7edfcfc on main.

[coveralls]

coverage: 94.431% (-0.004%) from 94.435% when pulling 72e0c97 on nimmo-matrix-box-carousels into d9687a8 on main.

[JoeCohen]

It's all good. (The obs_link works fine in the carousel.)
And it's better this way than jiust outside the carousel.
Very nice!

@JoeCohen ... aha, i figured out where I'd experimentally disabled it. The obs_link is back working now.

If you have time to check it out, please let me know if you think that's better or not. I do feel like people expect it at this point, but maybe not.

[JoeCohen]

As @nathan says, sweet.

[pellaea]

Overall I think this looks great, but since you asked, here is some criticism. I don't know how to do this in a way that doesn't sound like a litany of negative complaints, sorry. Best I could do is express them as requests and/or preferences. Hope that's good enough!

1. First thing I did was watch the logs. There are a whole bunch of identical simple queries, of two sorts:

CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162

and

CACHE Project Exists? (0.0ms)  SELECT 1 AS one FROM `projects` INNER JOIN `project_obser
vations` ON `projects`.`id` = `project_observations`.`project_id` WHERE `project_observati
ons`.`observation_id` = 515596 LIMIT 1

Since they are mostly repeats, they are cached and take < a millisecond, so maybe "who cares?" But this feels like something that could become important later when it may be less easy to catch. Or maybe it will go away on its own without us ever having to deal with. Clearly this is not ideal, but maybe not a problem. For now, just a note.

2. I see it rendering observations/namings/_propose_button.html.erb and observation_views/_mark_as_reviewed.html.erb each once for each matrix box... but I can't find out how to actually see these buttons! :) Either these partials are inaccessible in the UI and therefore should be removed, or the UI needs tweaking so that I can find them. I can't find them even though I know they should be there, haha! (Also, related, even though the partials take ca. 2 ms to load locally, and even multiplied by 100 this is still only 0.1-0.2 sec, it nevertheless may be worth considering in-lining or whatever the trick is you used to eliminate partials elsewhere that was such a resounding success.)

3. I notice that one big difference using the carousel is that you used to be able to click on the image to get to the observation page. But now, the only way to do that is by clicking on the mushroom name. I understand that clicking on the hot areas on left and right will now do something different. But is it really necessary (for technical or design reasons) to make clicking in the middle do nothing now? [Oh, I see this is specifically what you were asking re: hot areas. My experience agrees mostly with Joe's in that there is no hot area in the matrix box image which takes you to the observation page. The hot areas for the image votes, left and right carousel, and expando icon are all fine.]

4. I notice that when in expando-view, the carousel cycles through all of the observations and you can't tell where one observation ends and the next begins. (Well, okay, fine, if your window is big enough, you can see the text below the image, and if you pay close attention you can see it there, but it's subtle at best. This had me totally fooled for a while. It feels unnatural and surprising to me.) But the carousels in the main view do not. I would personally much prefer both expanded and main view carousels behave the same. And I would personally prefer the carousel showing only one observation at a time, and not wrapping around. Just my personal preference / vote. Having them behave the same is far more important than details of how they behave. (Although, of course, it makes no sense to have the carousels cycle through all of the observations in the main view!!) The wrap-around thing is even less important to me.

5. FWIW, I preferred the style of the image votes widget in the old version, where it extends all the way across the image and doesn't have rounded corners.

6. I notice also that clicking on the name in the main view takes you to the observation page, but clicking on the name in the expando view takes you to the name page. These should be the same, I think. In fact, I can't find any way to click through to the observation page from within the expando view, but maybe I'm missing it?

7. Ah! I see another difference. The new carousels enforce all the "cards" being the same size, whereas before we allowed the vertical-oriented images to be taller. Personally, I'm fine with this, just calling attention to the difference in case it's important to others. (I remember this being a point of considerable debate in the past!)

[pellaea]

PS. re: performance -- I just realized that my laptop blazes compared to the production server, so my back-of-the-envelope calculations leading me to conclude that those superfluous(?) cached queries are inconsequential may not be accurate estimates of real cost.

PPS. I'm running firefox 116.0.3 on ubuntu 5.15.0-82.

[mo-nathan]

I'm not sure which of my code you're referring to regarding count vs. length, but it wouldn't surprise me if I got it wrong somewhere. I looked through my last two PRs and didn't see anything obvious. However, the rule as I understand it is that length is Ruby and count is SQL. However, if you do something like namings.count { |n| n.name == name }.positive? it does not work well since it is running Rails code on something that should be handled in SQL. I believe the above will typically create and N+1 issue. I just tried Naming.all.count { |n| n.name == 'Russula' }.positive? and it started spewing tons of SQL (one for each Naming). However Naming.where(name: 'Russua').count just runs one query and quickly gives you the correct answer. I avoid Ruby procs in queries like the plague exactly because of issues like this. However, if you need the objects loaded into memory, then it totally make sense to do that in one query that hits SQL and then do any other counting and filter using Ruby to avoid additional queries.

As a side note, all of the above primarily applies to application code. Test code is a bit different. There may well be good reason to reconsult the database using count if the data may have been changed as a result of running the code in the test. In that case I lean towards hitting the database since it should really make no difference from a performance level and I'd rather be sure what I'm looking at is based on what got saved to the database since I've been burned way too many times due to caching when writing tests.

Does any of that address the question?

[mo-nathan]

Looking at the visual group code, the only place where I see .count getting used in application code is:

  def image_count(status = true)
    return visual_group_images.count if status.nil? || status == "needs_review"

    if status && status != "excluded"
      return visual_group_images.where(included: true).count
    end

    visual_group_images.where(included: false).count
  end

In this case I would argue that .count is appropriate because the function doesn't know whether visual_group_images has been loaded from SQL or if that data is expected to be used in this transaction. If it hasn't been used, then using length will load all that data into the server before counting it. Using count will just query the database and return a single number. In the cases I was able to find were image_count is called it is being called for all the images in a visual group, but the page only displays a small subset of those image. If you have a large visual group (like "Non-Diagnostic" in the "Big List" model which has more than 48,000 members), you don't want to load all that data just to get the count.

[nimmolo]

Thanks @mo-nathan.

That example you gave from observation.rb is a good one. Thinking what to do about this method:

  # Has anyone proposed a given Name yet for this observation?
  def name_been_proposed?(name)
    namings.count { |n| n.name == name }.positive?
  end

How about:

  # Has anyone proposed a given Name yet for this observation?
  def name_been_proposed?(name)
    namings.where(name: name).present?
  end

This seems pretty minimal.

You're right - the hits I found in your visual groups were a false positive, those should stay as they are.

Will add all this to a GitHub Discussion so it's available for reference.

[mo-nathan]

For:

  # Has anyone proposed a given Name yet for this observation?
  def name_been_proposed?(name)
    namings.count { |n| n.name == name }.positive?
  end

I find the alternative code you propsed to be clearer.

Testing it out, it does appear that the namings are already loaded, so your code does generate an extra query over the existing code. In this case I don't think that really matters since the amount of data is very small. If it did, I'd lean towards length rather than count (which also doesn't generate any additional queries in this case).

[nimmolo]

Thanks for the explanation. I will revert that change on the other PR then, because that PR is all about removing extra queries, even small ones.

[mo-nathan]

I tried this out and found that on my local o airport wifi, it was noticeably slower both using the developer tools and my own user experience. I think it went up from about 1.5 seconds to 2.5 seconds for the initial load and then the image loads felt a bit slower as well. For me the extra time didn't really feel worth it, but I expect I'd adapt to it. Unfortunately the internet is bad enough here that I can't really get more precise timing data.

[nimmolo]

@pellaea

1. First thing I did was watch the logs. There are a whole bunch of identical simple queries, of two sorts:

I definitely want to sort this out. This was the original thing that i noticed. Why are projects even getting loaded on this view?

2. I see it rendering observations/namings/_propose_button.html.erb and observation_views/_mark_as_reviewed.html.erb each once for each matrix box...

Need to fix!

3. I notice that one big difference using the carousel is that you used to be able to click on the image to get to the observation page.

This is restored now on the current branch of this PR.

4. I notice that when in expando-view, the carousel cycles through all of the observations and you can't tell where one observation ends and the next begins.

This was a longstanding request of Alan's. I don't mind it like this (i.e. as he requested), but it's easy to change.

5. FWIW, I preferred the style of the image votes widget in the old version, where it extends all the way across the image and doesn't have rounded corners.

Will check this, not sure it's showing like that for me.

6. I notice also that clicking on the name in the main view takes you to the observation page, but clicking on the name in the expando view takes you to the name page. These should be the same, I think. In fact, I can't find any way to click through to the observation page from within the expando view, but maybe I'm missing it?

Will consider adding a link to the obs in the lightbox.

[mo-nathan]

Was able to get some better data. On main I was seeing the initial page load at just over 2 seconds with subsequent loads at 1.6 seconds. On this branch the initial load was just over 3 seconds with subsequent loads around 2.6 seconds.

[pellaea]

@nimmolo -- I can take a closer look at the queries on the home page tomorrow maybe. I confess that I get a bit lost now that you've reorganized all of my code so thoroughly! ;) But I gotta learn what you've done eventually...

Also re: expando box scrolling continuously through all the observations -- I was thinking (for a future PR!) that one way to have the best of all worlds would be to have an animation of some sort "swipe" between images, and then add some sort of obvious spacer between the last image of the observation you're on and the first image of the next observation. That would be more than enough of a visual clue to keep me happy. I don't need much, just throw me the occasional crumb. ;) Just anything to signal to the viewer that they're moving on to another observation.

[mo-nathan]

I woke up way too early and couldn't get back to sleep, so I ran some more tests from my hotel room wifi. This branch is still consistently slower for me than main on the home page. Approximately 1.75x for the handful of pages I tested. I also noticed that this branch does a bunch more processing after the initial page load that can persist for up to a minute. It doesn't look like it's hitting the server at all, but it still makes me a bit nervous once we have bunch of folks hitting the same server.

I also noticed that on the branch there a bunch of addition redundant SQL queries that look like this:
CACHE Project Exists? (0.0ms) SELECT 1 AS one FROMprojectsINNER JOINproject_observationsONprojects.id=project_observations.project_idWHEREproject_observations.observation_id = 528117 LIMIT 1
It's happening between:
Rendered application/content/_sorter.html.erb (Duration: 0.4ms | Allocations: 380)
and
Rendered collection of shared/_matrix_box.html.erb within shared/_matrix_table [144 times] (Duration: 1749.0ms | Allocations: 4321331)

[nimmolo]

Ohhh... yep. I got it now. You've got an older DB snapshot, like Jason, I bet. I went back to the index near what's "current" for his snapshot, and yes I do get the weird extra queries.

  Rendered application/content/_sorter.html.erb (Duration: 0.2ms | Allocations: 172)
  Synonym Load (14.0ms)  SELECT `synonyms`.* FROM `synonyms` WHERE `synonyms`.`id` = 5702 LIMIT 1
  Name Load (245.7ms)  SELECT `names`.* FROM `names` WHERE `names`.`synonym_id` = 5702
   (1303.6ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.3ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  CACHE  (0.0ms)  SELECT COUNT(*) FROM `observations` WHERE `observations`.`name_id` = 59162
  Rendered collection of shared/_matrix_box.html.erb within shared/_matrix_table [24 times] (Duration: 3608.1ms | Allocations: 889628)

To me the suspect place might be my refactor of the show_obs_title and related helpers in helpers/observations_helper.rb. (I just fixed a problem with it and read through everything carefully. It seems right.) But i've got no clue about the projects queries and how that could possibly be related to the carousel — except for the image copyright computed in the lightbox caption by helpers/image_helper.rb

[nimmolo]

Aha. I believe the problem occurs in the lightbox printing the observation title with ALL the bells and whistles including preferred name link, owner naming, deprecated name...

And then there's the observation-details section below it, with so many links there too.

It was convenient to grab that partial for the lightbox caption, but maybe what we need instead is something like the obs details in plain text with no links, and a single link to the obs.

[nimmolo]

To reproduce the issue, go to http://localhost:3000/505457, and then hit "Index" to be sure we're on the same page of the index. Many extra queries will fire. If you examine the lightbox caption for that observation (it's encoded as a data-attribute in each image) you'll find the ID's of some of the associations that are firing the extra queries.

This is an issue in the lightbox currently, and it's being multiplied by having more images on the index. I'll try a separate PR to simplify the lightbox caption text, upstream, outside of this carousel PR.

[nimmolo]

Experimentally merging the lightbox caption PR into this to check if it helps.

[pellaea]

Yes! Simplify the lightbox caption! It would totally make sense that's where some of this project-related stuff is coming from. So glad you found that.

[nimmolo]

Ok. It does seem to fix it, but I will sit on this PR until I hear more from you guys about performance.

I'm amazed that Nathan can cold load the index in 3 or 4 seconds on main — on my machine it takes a minimum 25 seconds!

Consider also that Rails 7 will allow lazy loading of each matrix-box. That PR is my higher priority, and it may be that we will be happier with carousel performance in that case.

I'm also in no rush with this version of the carousels because i don't like the way it has to shrink the vertical images back to where they were a year ago. The Stimulus "swiper" carousel that's available on 7 can use variable height.

On the other hand, as Jason pointed out earlier, getting carousels up may save untold trips to show obs.

[mo-nathan]

I'm still seeing the weird queries on the latest version of nimmo-matrix-box-carousels. It's still slower especially if you count all the loads of the images. I've attached screenshots. I would guess the main reasons I'm seeing faster speeds are the M1 chip and not running in any sort of VM.

[mo-nathan]

Also as shown in the images, the database I'm running on is from September 7 and I have it configured to display 144 images.

[nimmolo]

I am unable to reproduce any weird query issue, and i've paged through the first 15 pages of 150 obs each on the current version of this branch, with a snapshot from 2023-03-02, the latest on the image server last week.

Note that many images are expected to keep loading after page load, because they're lazy loaded. Other assets seem to be getting lazyloaded too, like icons and arrows, and I don't know why that would be, but it's probably not different from main, nothing has changed in any config that I know of.

Nathan's using a newer snapshot than I have, though. I'm not sure what's different there. The screenshots show the image loads but not the queries.

[pellaea]

Hey, I just got done with what I was doing tonight. I'll take a look and see if I can track this down, because I was able to reproduce it earlier.

…

On Mon, Sep 11, 2023 at 11:48 PM andrew nimmo ***@***.***> wrote: I am unable to reproduce any weird query issue, and i've paged through the first 15 pages of 150 obs each on the current version of this branch. Note that images are expected to keep loading after page load, because in most cases they're lazy loaded. Other assets are getting lazyloaded too, like icons and arrows, and I don't know why that would be but it's probably not different on this branch. Nathan's using a newer snapshot than I have, though. I'm not sure what's different there. The screenshots don't show the queries — Reply to this email directly, view it on GitHub <#1598 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYTNNJUQTNSGSJGERA2Z5DXZ7LQ7ANCNFSM6AAAAAA4OHS4KY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[pellaea]

I found the problem with the projects. There are two things that require projects:

1. showing obs lat/long requires observations.projects to determine if the viewer has permission to see it
2. showing the original file name requires images.projects to determine if the viewer has permission to see it (note the need to eager-load both the observations' projects as well as the observations' images' projects -- we were missing the former)

I fixed it so that it doesn't need to eager-load observations.thumb_image. It's already eager-loading all of the images, so there's no reason to eager-load the thumbnail twice.

Lastly, I noticed that the carousels really slow down the page a lot. I debugged this by turning off the matrix boxes completely, then adding it back piece by piece. The eager-loading alone is only like 1 to 1.5 seconds on my laptop. But by time I re-enabled all of the carousel stuff it was up to about 3.5 seconds. This is all 100% rendering time, not database time. And no partials are involved either. Just pure, straight ruby. And lots of it. This has become very complex. Not sure how to fix it.

I'm pushing my changes now.

[nimmolo]

@pellaea thanks for checking this out so thoroughly. I will merge some of those last tweaks into main tomorrow, after examining more closely - they should probably go independently.

Question - do we need the image copyright info on the index lightbox? I knew this was a doozy of a multiple join. The question is, can we ditch it here? Cache it on a column?

This PR has certainly shed light if nothing else.

Might be a good opportunity to try out ViewComponents. I can make a PR off this branch turning the obs matrix box partial and helpers into VC's, and we compare speed then.

[mo-nathan]

I couldn't get VirtualBox working when I first got my M1. I tried Docker, but it increased the time it took for tests to run to something like 20 mins. I tweaked it a bit and it got a bit better, but never back to the level it had been using VirtualBox. I then ran into this weird RedCloth issue using Docker on M1, so I threw in the towel and just ran it natively. It's so fast, I haven't looked back. Another good metric that might be worth comparing is that for me the full test suite runs in under 2 minutes:

% rails test
Started with run options --seed 46145

  2307/2307: [==================================================================================================================] 100% Time: 00:01:48, Time: 00:01:48

Finished in 108.31886s
2307 tests, 27162 assertions, 0 failures, 0 errors, 0 skips
Coverage report generated for Minitest to /Users/nathan/src/mushroom-observer/coverage. 24539 / 26198 LOC (93.67%) covered.

[mo-nathan]

It looks like Jason's fix got rid of the weird queries. It's still noticeably slower (2.5 seconds vs. 1.5 seconds). The most noticeable thing is the image relead when I do a hard reload. On main it still must be using some sort of cache since it only adds about half a second. However, on the branch it takes a full minute to load the images for the 144 observations. All of that is browser time, so I'm a bit more concerned about the server load time, but I'd be ok with at least trying it out.

[pellaea]

Very interesting point about image loading. I thought we were lazy-loading image?

But the observation about the page being almost twice as slow is comparable to my own observation.

I don't think the extra includes (observation -> projects, images -> users, images -> projects) add significantly to the query time. Initially I ripped out all of the includes that I could, assuming that was going to help. But surprisingly, even after all of that I was still seeing the same ca. 3.5 second page load. Maybe a quarter of a second faster without several extra eager-loads? Not worth the extra complexity required to render different content on some pages than on other pages, in my opinion. So I just left those two "XXX consider dropping these on indexes" comments, but otherwise reverted as far as I could to Nimmo's original code. Because it was useful seeing where those projects were required.

[nimmolo]

Having these carousels is always going to be a tradeoff between avoiding round-trips to show obs, and slowing down the index itself. The carousels will always be slower, the question is if it's tolerable. I kind of feel like it is, although I'm not crazy about them.

However, maybe this should wait until i can rewrite the image partials as ViewComponents. That should speed up the index quite a lot — then maybe the carousels will not make the higher load time intolerable.