PTV-1620 - fix browser freeze and crash when rendering large heatmaps #3116

eapearson · 2022-10-20T13:37:02Z

Description of PR purpose/changes

This change fixes PTV-1620, which reflects a public ticket that cited two issues when viewing a FeatureCluster object

a data-tables error
browser freezing and/or crashing

The data-tables error was due to a missing field which was not supplied with a default value; data-tables does not like undefined values. This issue was fixed separately.

The changes in this PR fix the browser freezing and crashing issue. That problem is due to the creation of a heatmap when the number of features is greater than about 200. The heatmap is grows by N², where N is the number of features. The processing of the data for the heatmap beyond 200 features will begin to consume several seconds, and at a certain point may cause the browser to crash or report an unresponsive tab.

It is notable that the affected widget, kbaseExpressionPairwiseCorrelation, already restricted the display of a heatmap with over 50 features. With over 50 features in a cluster, it provides a link for downloading the heatmap, which is hidden.

However, it placed no limits on the creation of the heatmap. It is the creation of the heatmap that consumes a great deal of cpu and memory, leading to unresponsiveness and crashing.

This set of changes focuses on addressing this issue, but also:

corrects and improves some code issues within the same file.
fixes an unreported issue with downloading the heatmap (wherein the second and subsequent download attempts will fail.)

There are several UI and code issues that will be addressed by a subsequent PR, affecting quite a few additional files.

Jira Ticket / Issue

Jira Ticket https://kbase-jira.atlassian.net/browse/PTV-1620
Added the Jira Ticket to the title of the PR (e.g. DATAUP-69 Adds a PR template)

Testing Instructions

Unit tests have been added.

Tests pass locally and in GitHub Actions
Changes available by spinning up a local narrative and navigating to X to see Y

Dev Checklist:

My code follows the guidelines at https://sites.google.com/lbl.gov/trussresources/home?authuser=0
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
[-] I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
(JavaScript) I have run Prettier and ESLint on changed code manually or with a git precommit hook
[-] (Python) I have run Black and Flake8 on changed Python code manually or with a git precommit hook
[-] Any dependent changes have been merged and published in downstream modules

Updating Version and Release Notes (if applicable)

[-] Version has been bumped for each release
Release notes have been updated for each release (and during the merge of feature branches)

eapearson · 2022-10-20T14:29:42Z

The code analysis failure is due to the invocation of a kbwidget using "new" -- the widget itself is not stored, so the "bug" is constructing an object without using it. Just the weirdness of kbwidget freaking out code analysis.

codecov · 2022-10-20T15:20:21Z

Codecov Report

Merging #3116 (fa0491e) into develop (a198ed2) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3116   +/-   ##
========================================
  Coverage    73.37%   73.37%           
========================================
  Files           36       36           
  Lines         3917     3917           
========================================
  Hits          2874     2874           
  Misses        1043     1043

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21d9e03...fa0491e. Read the comment docs.

eapearson · 2022-10-20T16:19:15Z

I'll have a closer look at the failing test ... it works locally.

eapearson · 2022-10-24T00:52:03Z

Added ticket https://kbase-jira.atlassian.net/browse/PTV-1844 because I wanted to have screenshots of each of the widget states.

It may be reviewed directly by building off this PR or the branch and viewing a FeatureClusters object, e.g. as output from "Cluster Expression Data - K-Means"

briehl

Just some minor test file tweaks, but looks great. Thanks for doing this!

briehl · 2022-10-24T15:28:46Z

test/unit/spec/util/asyncTools.js

+     * @param {int} duration
+     * @returns {Promise<void>}
+     */
+    function waitFor(duration) {


This is effectively a duplicate of https://github.com/kbase/narrative/blob/develop/test/unit/testUtil.js#L63 and should just use that one.

The tryFor function could probably live in testUtil as well. At the least, this file should be moved out of the test/spec/util directory and up into the should be test directory, as it's not a test spec.

Yeah, I think this was done a long time ago in the original PR, and I was trying to make as few changes from that as possible. E.g. there was nothing else in the util directory then, just mwsTool.js!
Knowing me, I also wanted the tests to be low-dependency, and it seemed like there as a lot going on in testUtils...
Anyway, I'm happy to move the code out and into testUtils (I had moved the non-spec files out myself during testing so I could isolate tests w/o the util specs.)

Okay, have the changes locally, will update the PR a bit later this afternoon.

So the changes are up. There were changes to a dozen or so files to enable this, and I included some commented-out test config features that I've found useful for focusing tests and reducing test output clutter.
The mswUtils was moved into a unit/utils directory to clarify purpose and ease inclusion of test utilities (e.g. don't need to individually add that file, don't need to individually map it for requires). asyncUtils was removed and the non-duplicate function (tryFor) moved into testUtils.

briehl · 2022-10-24T15:32:51Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseExpressionPairwiseCorrelation.js

+                // Theoretically this ensures that the link and url are not needed any more.
+                // I don't know of documentation for this behavior, but it does seem to work.
+                // Note that there is nothing really to "download", as the heatmap content is
+                // already loaded in the browser.


briehl · 2022-10-24T15:34:26Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseExpressionPairwiseCorrelation.js

+                // I don't know of documentation for this behavior, but it does seem to work.
+                // Note that there is nothing really to "download", as the heatmap content is
+                // already loaded in the browser.
+                setTimeout(() => {


Is the setTimeout wrapper bit necessary? Is that something to do with the Blob lifecycle?

Just added this to the comment:

I didn't find documentation for this, but I figure that the click is
not handled until the next turn of the wheel in the JS event loop. So the
timeout just lets the event loop handle the click, and timers are executed
after any pending events, so this should ensure that the link being clicked
is not removed until the link 'click' is handled.

… usage of waitFor/wait, add some commented-out useful test settings, and adjust affected tests [PTV-1620]

…tory [PTV-1620]

sonarqubecloud · 2022-10-25T00:00:35Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

briehl · 2022-10-25T16:13:15Z

Thanks for the changes. This works for me.

PTV-1620 fix usage of $('<tag>') in PR #3116

eapearson · 2022-12-19T17:41:59Z

Just adding a comment to note that this implements a sub ticket of PTV-1620: PTV-1844

fix browser freeze and crash when rendering large heatmaps

aea4f59

eapearson changed the title ~~fix browser freeze and crash when rendering large heatmaps~~ PTV-1620 - fix browser freeze and crash when rendering large heatmaps Oct 20, 2022

eapearson requested a review from briehl October 20, 2022 14:29

add tests

79e6aee

add tests utility

f9b75b2

remove newline in content (breaks a test) [PTV-1620]

0913d23

briehl requested changes Oct 24, 2022

View reviewed changes

eapearson mentioned this pull request Oct 24, 2022

PTV-1620/PTV-1845 fix kbaseTabs #3117

Open

10 tasks

eapearson added 3 commits October 24, 2022 13:07

remove newline in content (breaks a test) [PTV-1620]

3be820f

merge asyncTools into testUtils, move mswUtils, remove some duplicate…

5b805b1

… usage of waitFor/wait, add some commented-out useful test settings, and adjust affected tests [PTV-1620]

adjust volume mount to accommodate (avoid) the new ext_packages direc…

fa0491e

…tory [PTV-1620]

briehl self-requested a review October 25, 2022 16:12

briehl approved these changes Oct 25, 2022

View reviewed changes

briehl merged commit 0688db9 into develop Oct 25, 2022

briehl deleted the PTV-1620-browser-freeze branch October 25, 2022 16:13

eapearson mentioned this pull request Oct 26, 2022

PTV-1620 fix usage of $('<tag>') in PR #3116 #3119

Merged

10 tasks

briehl added a commit that referenced this pull request Oct 26, 2022

Merge pull request #3119 from kbase/PTV-1620-browser-freeze-part-2

afd5b14

PTV-1620 fix usage of $('<tag>') in PR #3116

eapearson mentioned this pull request Jan 3, 2023

fix kbaseTabs behaving incorrectly when a deletable tab is deleted [PTV-1845] #3174

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PTV-1620 - fix browser freeze and crash when rendering large heatmaps #3116

PTV-1620 - fix browser freeze and crash when rendering large heatmaps #3116

eapearson commented Oct 20, 2022 •

edited

Loading

eapearson commented Oct 20, 2022

codecov bot commented Oct 20, 2022 •

edited

Loading

eapearson commented Oct 20, 2022

eapearson commented Oct 24, 2022

briehl left a comment

briehl Oct 24, 2022

eapearson Oct 24, 2022

eapearson Oct 24, 2022

eapearson Oct 25, 2022

briehl Oct 24, 2022

briehl Oct 24, 2022

eapearson Oct 24, 2022

sonarqubecloud bot commented Oct 25, 2022

briehl commented Oct 25, 2022

eapearson commented Dec 19, 2022

PTV-1620 - fix browser freeze and crash when rendering large heatmaps #3116

PTV-1620 - fix browser freeze and crash when rendering large heatmaps #3116

Conversation

eapearson commented Oct 20, 2022 • edited Loading

Description of PR purpose/changes

Jira Ticket / Issue

Testing Instructions

Dev Checklist:

Updating Version and Release Notes (if applicable)

eapearson commented Oct 20, 2022

codecov bot commented Oct 20, 2022 • edited Loading

Codecov Report

eapearson commented Oct 20, 2022

eapearson commented Oct 24, 2022

briehl left a comment

Choose a reason for hiding this comment

briehl Oct 24, 2022

Choose a reason for hiding this comment

eapearson Oct 24, 2022

Choose a reason for hiding this comment

eapearson Oct 24, 2022

Choose a reason for hiding this comment

eapearson Oct 25, 2022

Choose a reason for hiding this comment

briehl Oct 24, 2022

Choose a reason for hiding this comment

briehl Oct 24, 2022

Choose a reason for hiding this comment

eapearson Oct 24, 2022

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 25, 2022

briehl commented Oct 25, 2022

eapearson commented Dec 19, 2022

eapearson commented Oct 20, 2022 •

edited

Loading

codecov bot commented Oct 20, 2022 •

edited

Loading