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

Improve Profile Viewer performance & user experience #114

Merged
merged 7 commits into from
Jul 16, 2021
Merged

Conversation

Bronstrom
Copy link
Contributor

@Bronstrom Bronstrom commented Jul 14, 2021

Due to lengthy rendering, the Profile Viewer takes a long period of time to show elements on the page. In order to improve the user experience, a skeleton framework has been added to display on the page while rendering the resolved controls. To improve performance, a few areas of OSCALProfile, OSCALControl, & underlying components have been optimized. Most notably, the modifications data sent between components have been slimmed down.

During the page loading process in Profile Viewer, a Skeleton
placeholder is added as a visual indicator. The added placeholder is
modeled after the card structure of the imported controls. Additional
documentation has been included in Profile as well.
These create a new function every time components are re-rendered. To
optimize these methods, the 'function' keyword is used instead.
Some of the Control modification attributes didn't need to be
included when rendering controls and have been removed. Some other
minor areas of control modifications have been improved.
To resolve a browser error, a key has been given to the skeleton
cards. The ControlProse test suite was failing due to the change
in how modifications are handled, and now has been updated.
@Bronstrom
Copy link
Contributor Author

I wanted to note the live sandbox environment seemed to already run the FedRAMP moderate profile in about 2 or less seconds (as desired in the Acceptance Criteria). There seems to be a performance drop with it locally, especially when using the reload button after a few times. I was able to shave a couple seconds off when running it locally - which I think will still show a noticeable change on AWS.

Copy link
Contributor

@kkennedy26 kkennedy26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I got this error when running locally and clicking to the profile viewer.

@Bronstrom
Copy link
Contributor Author

I got this error when running locally and clicking to the profile viewer.

@kkennedy26 I was using the FedRAMP profile as the default profile when testing. I just realized the NIST profile doesn't have set-parameters. I'll make a fix for this.

@Bronstrom Bronstrom self-assigned this Jul 15, 2021
@kylelaker
Copy link
Contributor

This looks great and I am really excited about the potential improvements here! There are a lot of changes. Can you talk about how specifically this was optimized? Are there performance benefits to function over const X = () => {}?

@Bronstrom
Copy link
Contributor Author

This looks great and I am really excited about the potential improvements here! There are a lot of changes. Can you talk about how specifically this was optimized? Are there performance benefits to function over const X = () => {}?

Of course - I was a bit vague about optimization. The arrow functions (const X = () => {}) create a new function every time components are re-rendered. To avoid this, the "function" keyword can be used instead. Arrow functions are apparently good in less dynamic contexts. For more information see: 5 Ways to Optimize Your Functional React Components - Avoid arrow functions when possible and When (and why) you should use ES6 arrow functions. I also made the choice to split up the modification attributes, set-parameters from alters, to lessen the amount of data shared between underlying components. This should help with the efficiency of OSCALControlProse and OSCALControlModifications, which are doing a lot of the work.

@kkennedy26
Copy link
Contributor

Looks good. Can definitely see the improvements in load time with Profile Viewer since the recent commit

@Bronstrom
Copy link
Contributor Author

Bronstrom commented Jul 16, 2021

To further improve performance, I had the idea that more data could be filtered out of modifications (it appears to be what's taking the longest time to load and render), such as getting rid of the removes attribute in modify.alter. Although, we will probably be using this data soon in a future issue (EGRC-407). I was also looking into useMemo() and useCallback(), but there doesn't seem to be a function that's being repeatedly rendered in the same exact way (like with how the control ID parameter is unique for rendering the majority of functions) or they would minimally help. The functions I attempted these on seemed to have a lot of overhead causing the Profile Viewer to stagnate more. Let me know if anyone has a different opinion or guidance on this.

Copy link
Contributor

@kkennedy26 kkennedy26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized I didn't approve this in my previous comment but meant to haha.. good work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Profile Viewer Performance/Loading Display
3 participants