Tooltips cause grid slowness

Bug #1797007 reported by Jeff Davis on 2018-10-10
30
This bug affects 6 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned
3.0
Medium
Unassigned
3.1
Medium
Unassigned

Bug Description

EG 3.1

After applying the changes for bug 1740412 and bug 1794176 (tooltips) in our EG 3.1.4 environment, we found that web client grids were substantially slower to load. The effect was noticeable in checkin, checkout, patron items out, and patron search. After reverting the tooltip changes, grid performance returned to the pre-tooltip baseline, i.e. 3-4 seconds rather than 10+ seconds for certain functions.

To confirm the problem, we tested on a 3.1.6 test system with concerto data. Loading the Items Out tab for a patron with 20+ items out took 10+ seconds for the grid to fully load. (Grid rows appear in chunks; the first chunk appears in about 5 seconds, then there's a further delay before the second chunk.) Checkouts also took 10+ seconds for full grid refresh after 20 items or so. And we saw similar delays on checkin, although bug 1777207 may be a confounding factor there.

Jeff Davis (jdavis-sitka) wrote :

On our 3.1.6 test system, we tried reverting just the template change for bug 1794176. This improved grid refresh times by a few seconds, but the grid seemed still noticeably slower overall than the pre-tooltip baseline.

Jason Stephenson (jstephenson) wrote :

I'm marking this confirmed because we see this same behavior at CW MARS after our upgrade to Evergreen 3.0.12 this past weekend. We were seeing checkin taking anywhere from 3 to 10 seconds. After reverting the five commits that implement the tooltip updates, checkin times returned to what was normal before the upgrade.

I made a list of the commits to revert for anyone who wants immediate relief before a fix goes in:

https://pastebin.com/NHM0PNFh

We have seen the performance degradation from bug 1777207 since before our upgrade, so this is definitely something different.

Changed in evergreen:
status: New → Confirmed
Bill Erickson (berick) wrote :

Likely the changes in bug #1794176 are the main culprit. To fix stale grid values, the cell content and tooltip content are generated independently and they respond to all Angular digests, which can mean multiple re-renderings for each, instead of sharing a single cached value. There may be ways to optimize this without losing the bug fix.

On a related note, before we added tooltips, I added a feature to the Ang6 grid that would allow users to vertically expand the grid so all content became visible at the expense of occupying more vertical space. You can see an example in the Acq admin interfaces, for example:

https://HOSTNAME/eg2/en-US/staff/admin/acq/lineitem_marc_attr_definition

See the grid toolbar action button second from top right (arrow down) with tooltip "Expand Cells Vertically". When clicked, the XPATH values, which typically extend beyond the cell, become fully visible as the rows expand vertically.

This could potentially replace tooltips if users agree it's a suitable replacement.

Kathy Lussier (klussier) wrote :

I'm not going to offer an opinion on replacing tooltips, Bill, but I wanted to mention that when I try the vertical expand option on demo.evergreencatalog.com, it doesn't always show the entire cell contents. See the xpath in the first row of the attached screenshot.

I assume it's related to my resolution, monitor size, etc.

Jeff Davis (jdavis-sitka) wrote :

I'd really like to keep tooltips if we can avoid the performance hit. It's a great feature. Expanding cells vertically sounds useful too, but not ideal for long unbroken strings like barcodes.

Michele Morgan (mmorgan) wrote :

I agree with Jeff, I'd hate to lose the tooltips. They're a significant enhancement to usability in busy grids.

Bill Erickson (berick) wrote :

Good point about unbroken strings, vertical expansion won't help there, since they don't wrap.

Bill Erickson (berick) wrote :

Or maybe they can.. Note to self, improve vertical expansion of unbroken text with CSS along the lines of:

word-break: break-all; word-wrap: break-word;

Jason Stephenson (jstephenson) wrote :

The fix for 3.0 is to revert the commits. This should probably not have been backported in the first place.

Jason Boyer (jboyer) wrote :

The reason that the changes in bug #1794176 are causing issues is that every single call to cellOverflowed() causes a (full page?) re-layout, and it is called at a minimum $cols times when adding a new row to a grid. ($rows * $cols for checkin. :( ) I checked in a single item into a fresh checkin page and looking at my Chrome Performance tab shows that 800+ms is spent in the angular $digest function, most of which is actually cellOverflowed() waiting for Chrome's Layout action. There's a very large number of calls in general and most are a few ms here, a couple us there, but the time spent waiting on Layout far outweighs everything else in that near-second per row. (second checkin presented me with a 1.3+s $digest...)

I'm not sure of the best way to mitigate all of these delays since the simple act of requesting an element's size is the problem (possibly each of the 4 accesses? I don't know that for sure) and we really need to know the size of the columns (can be cached easily enough at times) and the elements within them (all different... :( ).

Jason Boyer (jboyer) wrote :

I should have included in my last comment that as a test I just hard-coded cellOverflowed() to always return true and that basically brought things back to pre - bug #1794176 speeds, but with the fixes for blank rows. (The Performance tab showed far less time in Layout and $digest was only ~160ms for the first checkin and ~280 for the second. I don't think having tooltips for values narrower than the column is so bad a tradeoff (and can be seen in other applications here and there).

Does anyone see an issue with that solution? (Other than the obvious "just throw the check away if it only ever returns true")

Bill Erickson (berick) wrote :

Branch pushed which rolls back the content width-dependent logic for tooltip display in grids, which avoids the time consuming page re-flowing.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1797007-grid-tooltips-always-show

tags: added: pullrequest webstaffclient
Bill Erickson (berick) wrote :

Note the same must be done for ang6 grids. Removing pulling request until that patch is added.

tags: removed: pullrequest
Bill Erickson (berick) on 2018-11-06
Changed in evergreen:
assignee: nobody → Bill Erickson (berick)
Bill Erickson (berick) wrote :

Second commit pushed with same logic changes to the Angular grid.

tags: added: pullrequest
Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
Jason Boyer (jboyer) wrote :

I've loaded Bill's branch and poked around at AngJS and Ang grids and the speed was good. Signoff is here: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1797007-grid-tooltips-signoff working/user/jboyer/lp1797007-grid-tooltips-signoff

I have located a gap in my git-fu and my signoff is only on the latest commit. Rest assured I've looked at both and signoff thereto and whetherfor, etc.

Andrea Neiman (aneiman) on 2018-11-14
tags: added: signedoff
Galen Charlton (gmc) on 2018-11-15
Changed in evergreen:
milestone: none → 3.2.2
no longer affects: evergreen/3.2
Changed in evergreen:
assignee: nobody → Galen Charlton (gmc)
importance: Undecided → Medium
Galen Charlton (gmc) wrote :

Pushed to master, rel_3_2, and (the AngularJS patch to) rel_3_1. Thanks, Bill and Jason!

Changed in evergreen:
assignee: Galen Charlton (gmc) → nobody
status: Confirmed → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers