Skip to content
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

Opening the hierarchy for a concept takes a long time #1377

Closed
joelit opened this issue Oct 11, 2022 · 10 comments · Fixed by #1383
Closed

Opening the hierarchy for a concept takes a long time #1377

joelit opened this issue Oct 11, 2022 · 10 comments · Fixed by #1383
Assignees
Milestone

Comments

@joelit
Copy link
Contributor

joelit commented Oct 11, 2022

At which URL did you encounter the problem?

https://finto.fi/yso/en/page/p28785

What steps will reproduce the problem?

  1. Load the page with the network inspector open
  2. The page content loads in a reasonable time, but the hierarchy takes a long time to open
  3. This is shown in the network inspector as the time to finish, for example:
  • Chrome: Finish: 5.75 s DOMContentLoaded: 551 ms Load: 5.67 s
  • Firefox: Finish: 7.06 s DOMContentLoaded: 583 ms load: 1.01 s
  • Edge: Finish: 9.97 s DOMContentLoaded: 691 ms Load: 9.78 s
    (Firefox seems to interpret load time differently to Chrome and Edge)

Screenshot from 2022-10-11 17-49-48

What is the expected output? What do you see instead?

This lag time seems to be consistent to certain concept pages. My initial hunch was that this would be proportional to the depth of the concept in the hierarchy tree, but that doesn't seem to be the case. Something like https://finto.fi/yso/en/page/p7873 has more than double the hierarchy depth while keeping the time to finish much shorter.

What browser did you use? (eg. Firefox, Chrome, Safari, Internet explorer)

Firefox, Chrome & Edge

@joelit joelit added this to the Next Tasks milestone Oct 11, 2022
@osma
Copy link
Member

osma commented Oct 26, 2022

How long did the call to the hierarchy REST method take? I'm wondering whether most of the time is spent in the REST call to the backend, or in the JS frontend (e.g. jsTree). I assume it's the frontend that is slow, but it wasn't really clear from the report.

Also, do we know whether this is a regression or has the hierarchy display always been as slow for this and similar concepts? If it's a regression, a plausible reason could be the recent jQuery upgrade.

@kinow
Copy link
Collaborator

kinow commented Oct 26, 2022

How long did the call to the hierarchy REST method take? I'm wondering whether most of the time is spent in the REST call to the backend, or in the JS frontend (e.g. jsTree). I assume it's the frontend that is slow, but it wasn't really clear from the report.

I was curious about that too, so I had a look at the Network tab in Firefox. Three XHR requests finish fairly quickly (~1, 2 seconds):

image

Looking at Firefox profiler (fancy, it now uses profiler.firefox.com) it shows the code taking a long time to run some natural sorting, which would be a good explanation for the slowness:

image

The natural sort function:

// Natural sort from: http://stackoverflow.com/a/15479354/3894569
// adapted to return only -1 or 1 using negVsPos function above
function naturalCompare(a, b) {
var ax = [], bx = [];
a.replace(/(\d+)|(\D+)/g, function(_, $1, $2) { ax.push([$1 || Infinity, $2 || ""]); });
b.replace(/(\d+)|(\D+)/g, function(_, $1, $2) { bx.push([$1 || Infinity, $2 || ""]); });
while(ax.length && bx.length) {
var an = ax.shift();
var bn = bx.shift();
var nn = (an[0] - bn[0]) || an[1].localeCompare(bn[1], lang);
if(nn) return negVsPos(nn);
}
return negVsPos(ax.length - bx.length);
}

Maybe someone could try playing with the sorting configuration, or hack the code to remove the natural sort, and see if that would perform better:

// sort on notation if requested, and notations exist
if (window.sortByNotation) {
var aNotation = aNode.original.notation;
var bNotation = bNode.original.notation;
if (aNotation) {
if (bNotation) {
if (window.sortByNotation == "lexical") {
if (aNotation < bNotation) {
return -1;
}
else if (aNotation > bNotation) {
return 1;
}
} else { // natural
return naturalCompare(aNotation, bNotation);
}
}
else return -1;
}
else if (bNotation) return 1;
// NOTE: if no notations found, fall back on label comparison below
}
// no sorting on notation requested, or notations don't exist
return naturalCompare(nodeLabelSortKey(aNode), nodeLabelSortKey(bNode));
}

@mirmaid
Copy link
Collaborator

mirmaid commented Oct 26, 2022

For users it's a problem that while the hierarchy loads, links and copy-icon do not work, and when the hierarchy has finished loading, the concept view "moves", which is distracting as you have already started to focus on something on it or trying to work with it. Also, if you try to click something several times, you can occasionally break the hierarchy somehow (dotted lines disappear).

@kinow
Copy link
Collaborator

kinow commented Oct 26, 2022

the concept view "moves", which is distracting as you have already started to focus on something on it or trying to work with it.

I also find it distracting when that happens on web or mobile (I have an Android app that does that). The name in UIUX is “Content Shifting”. There's a whole lot of articles about it and techniques to solve it (those elements that display a loader or several lines animated as if they are loading something, called skeletons in some UI frameworks), e.g. : https://www.smashingmagazine.com/2016/08/ways-to-reduce-content-shifting-on-page-load/

@osma
Copy link
Member

osma commented Oct 26, 2022

Thanks for the quick analysis @kinow ! Indeed, the natural sort function seems like a probable culprit here.

the concept view "moves", which is distracting as you have already started to focus on something on it or trying to work with it.

This is partly an unfortunate side effect of the way we reimplemented the custom scrollbar functionality in the latest Skosmos release. When we switched away from the old and unmaintained mCustomScrollbar library, we had to also reimplement the centering of the hierarchy so that the currently selected concept is visible in the hierarchy when you open a concept page. I did that in PR #1372 (and tweaked in #1374) using the standard Element.scrollIntoView method. But this may scroll the entire page in some cases, not just the hierarchy view which was intended. I think we need a proper issue report on this and a separate fix which is unrelated to the performance problem in this issue, although their combined effect (first waiting ~10s for the hierarchy to render, then the auto-scroll happens) is especially bad for usability.

@osma
Copy link
Member

osma commented Oct 26, 2022

@kinow I think it's not necessary the naturalSort function that is slow here (although it could be), but the related function nodeLabelSortKey - this can be seen also in your profiler screenshot, on the last line of the call tree. This one:

function nodeLabelSortKey(node) {
// make sure the tree nodes with class 'domain' are sorted before the others
// domain will be "0" if the node has a domain class, else "1"
var domain = (node.original.a_attr['class'] == 'domain') ? "0" : "1";
// parse the HTML code in node.text and return just the label as a lower case value for sorting
// should look like '<span class="tree-notation">12.3</span> <span class="tree-label">Hello</span>'
var label = $($.parseHTML(node.text.toLowerCase())).filter('.tree-label').text();
return domain + " " + label;
}

It was introduced in PR #1205 (by yours truly) and intended to extract just the label (not the notation) from the jsTree node representing a concept in the hierarchy. But it performs HTML parsing with jQuery, and I can see now that it could be very slow. Probably we need another, faster mechanism for extracting the label.

@miguelvaara
Copy link
Contributor

miguelvaara commented Oct 26, 2022

@kinow @osma, I think the simplest way to reproduce the problem related to the vertical centering of the hierarchy is to come to the page with a direct link, like this: https://finto.fi/ykl/en/page/99.19 (success maybe depends on your screen resolution or window size). By clicking on the hierarchy tree, the problem is hard to reproduce.

@osma
Copy link
Member

osma commented Oct 26, 2022

@miguelvaara that problem (separate from this performance issue) is now reported as issue #1382

@kinow
Copy link
Collaborator

kinow commented Oct 26, 2022

It was introduced in PR #1205 (by yours truly) and intended to extract just the label (not the notation) from the jsTree node representing a concept in the hierarchy. But it performs HTML parsing with jQuery, and I can see now that it could be very slow. Probably we need another, faster mechanism for extracting the label.

😄 I suspected that it would be either the natural sort, maybe moving that to the server side. But looks like it's a bit trickier. I hadn't seen that it was parsing HTML. I think you are correct, and on the right path to fix it.

@osma osma self-assigned this Oct 26, 2022
@osma
Copy link
Member

osma commented Oct 26, 2022

Yes, it's definitely the HTML parsing. I tried shortcutting it and now the hierarchy loads much faster.

@osma osma modified the milestones: Next Tasks, 2.17 Oct 26, 2022
osma added a commit that referenced this issue Nov 17, 2022
@joelit joelit moved this to Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR) in Skosmos 3.0 Backlog Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging a pull request may close this issue.

5 participants